Project

General

Profile

Actions

Bug #4573

closed

Remove unused variables ADMIN, POLICYCHILDREN and CHILDRENID from "common" system technique metadata

Added by François ARMAND over 10 years ago. Updated over 10 years ago.

Status:
Released
Priority:
N/A
Category:
System techniques
Target version:
Severity:
UX impact:
User visibility:
Effort required:
Priority:
Name check:
Fix check:
Regression:

Description

I'm not sur about the version for which this is the case, it should match the change done for relay servers.

I'm not sur about ADMIN, I believe it was already removed in 2.10.


Subtasks 3 (0 open3 closed)

Bug #4574: Remove ADMIN from common metadataRejectedNicolas CHARLES2014-03-07Actions
Bug #4575: Remove unused datas from the LDAP tree initialisationReleasedFrançois ARMAND2014-03-07Actions
Bug #4576: Remove ADMIN from the LDAP treeReleasedFrançois ARMAND2014-03-07Actions

Related issues 1 (0 open1 closed)

Related to Rudder - User story #4033: Add system variables filling MANAGED_NODES_NAME and MANAGED_NODES_ID in RudderReleasedFrançois ARMAND2013-10-09Actions
Actions #1

Updated by Nicolas CHARLES over 10 years ago

POLICYCHILDREN and CHILDRENID were removed in 2.8, ADMIN in 2.10.
I'm openin another ticket for ADMIN

Actions #2

Updated by Nicolas CHARLES over 10 years ago

  • Target version set to 2.8.4
Actions #3

Updated by Nicolas CHARLES over 10 years ago

  • Status changed from 8 to Pending technical review
  • Assignee changed from Nicolas CHARLES to Jonathan CLARKE
  • Pull Request set to https://github.com/Normation/rudder-techniques/pull/312
Actions #4

Updated by Nicolas CHARLES over 10 years ago

  • Status changed from Pending technical review to In progress

ha, it is a bit more complex than that, as it fails at deployment

Actions #5

Updated by Nicolas CHARLES over 10 years ago

  • Assignee changed from Jonathan CLARKE to Nicolas CHARLES
Actions #6

Updated by Nicolas CHARLES over 10 years ago

  • Status changed from In progress to Pending technical review
  • Assignee changed from Nicolas CHARLES to Jonathan CLARKE

Ok, it doesn't fail, it just complains in the log that some variables cannot be stored, but it works

Actions #7

Updated by Jonathan CLARKE over 10 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from Jonathan CLARKE to François ARMAND

I don't understand this ticket. Removing stuff is not a bug. What is the actual bug (what doesn't work or is broken here?). Please clarify.

Actions #8

Updated by François ARMAND over 10 years ago

Having dead code is a bug:

- it may be call in unwanted or unknown way, leading to bigger problem (security pb) and/or extremely hard bug to reproduce
- it may send bad message to future mainteners, thinking that something is used but is actually not.

That's of course generalities, but in that precise case, these variables were still present in technique metadata but not used for the promise generation, so we add a desynchronisation between technique metadata and actual variable, and we were paying a non negligible processing time for them for nothing.

Moreover, they were the only remaining use cases of the ${rudder.ruleid.target.XXX} parameter kind, blocking a possible major simplification (see #4585)

Actions #9

Updated by François ARMAND over 10 years ago

  • Status changed from Discussion to Pending technical review
  • Assignee changed from François ARMAND to Jonathan CLARKE
Actions #10

Updated by Jonathan CLARKE over 10 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from Jonathan CLARKE to François ARMAND

François ARMAND wrote:

Having dead code is a bug:

- it may be call in unwanted or unknown way, leading to bigger problem (security pb) and/or extremely hard bug to reproduce
- it may send bad message to future mainteners, thinking that something is used but is actually not.

That's of course generalities, but in that precise case, these variables were still present in technique metadata but not used for the promise generation, so we add a desynchronisation between technique metadata and actual variable, and we were paying a non negligible processing time for them for nothing.

Moreover, they were the only remaining use cases of the ${rudder.ruleid.target.XXX} parameter kind, blocking a possible major simplification (see #4585)

Thanks for the clarification.

I agree this needs removing, however, in the interest of stability, we must avoid making unnecessary changes, and in particular any changes that don't fix a problem suffered by users in released branches. Can we retarget this to 2.10?

Actions #11

Updated by François ARMAND over 10 years ago

  • Status changed from Discussion to Pending technical review
  • Assignee changed from François ARMAND to Jonathan CLARKE

Well, they should have been removed along with the introduction of MANAGED_NODES_NAME and MANAGED_NODES_ID in #4033 (so in 2.8), but I'm ok to remove them only in 2.10.

Actions #12

Updated by Jonathan CLARKE over 10 years ago

  • Status changed from Pending technical review to Discussion
  • Assignee changed from Jonathan CLARKE to Nicolas CHARLES

François ARMAND wrote:

Well, they should have been removed along with the introduction of MANAGED_NODES_NAME and MANAGED_NODES_ID in #4033 (so in 2.8), but I'm ok to remove them only in 2.10.

Great. Assigning to Nicolas to retarget the PR.

Actions #13

Updated by Jonathan CLARKE over 10 years ago

François ARMAND wrote:

Having dead code is a bug:

Oh, and to respond to this, let me clarify what my problem here is: maybe having dead code is a bug, but the title of this ticket never mentions dead code. It is expressed exactly like a "Let's remove some stuff" ticket which sounds scary, whereas it should be more like a "Let's clean up this unused stuff" which sounds good.

Titles matter. A lot.

Actions #14

Updated by François ARMAND over 10 years ago

  • Target version changed from 2.8.4 to 2.10.0~beta1
Actions #15

Updated by François ARMAND over 10 years ago

  • Status changed from Discussion to In progress

Nico, could you retarget the associated PR (and other for the children) to 2.10 (given all the discussion above ?)

Thanks !

Actions #16

Updated by Nicolas CHARLES over 10 years ago

  • Pull Request changed from https://github.com/Normation/rudder-techniques/pull/312 to https://github.com/Normation/rudder-techniques/pull/317
Actions #17

Updated by Jonathan CLARKE over 10 years ago

  • Subject changed from Remove ADMIN, POLICYCHILDREN and CHILDRENID from "common" system technique metadata to Remove unusued variables ADMIN, POLICYCHILDREN and CHILDRENID from "common" system technique metadata

Clarifying the title to make it actually sound like a bug!

Actions #18

Updated by Nicolas CHARLES over 10 years ago

  • Status changed from In progress to Pending release
  • % Done changed from 67 to 100

Applied in changeset policy-templates:commit:6f23559579946857d0bda5fd6c252e926f4c264d.

Actions #19

Updated by Anonymous over 10 years ago

Applied in changeset policy-templates:commit:cec1a3bd22ad95414386b2b935fdf1a8dc37eb16.

Actions #20

Updated by Vincent MEMBRÉ over 10 years ago

  • Subject changed from Remove unusued variables ADMIN, POLICYCHILDREN and CHILDRENID from "common" system technique metadata to Remove unused variables ADMIN, POLICYCHILDREN and CHILDRENID from "common" system technique metadata
Actions #21

Updated by Vincent MEMBRÉ over 10 years ago

  • Status changed from Pending release to Released

This bug has been fixed in Rudder 2.10.0~beta1, which was released today.
Check out:

The release announcement: http://www.rudder-project.org/pipermail/rudder-announce/2014-March/000084.html
The full ChangeLog: http://www.rudder-project.org/foswiki/bin/view/System/Documentation:ChangeLog210
Download information: https://www.rudder-project.org/site/get-rudder/downloads/
Actions

Also available in: Atom PDF