Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixes #18302: Make possible to give a revision for directives in rule and get correct technique files #3247

Merged
merged 1 commit into from
Jul 26, 2021
Merged

Fixes #18302: Make possible to give a revision for directives in rule and get correct technique files #3247

merged 1 commit into from
Jul 26, 2021

Conversation

fanf
Copy link
Member

@fanf fanf commented Oct 7, 2020 •
edited
Loading

https://issues.rudder.io/issues/18302

First version that allows to use past revisions of Techniques and Directives in rule (and directly from directive page by adding +commid near directive UUID.

This is still WIP as we need to have API for directive (and unit tests) allowing to get a specific revision, and set revision for directives in rules (API to list revisions, clone a specific revision, create or clone on a branche different from master will be done in next steps, along with revision for rules).

Since first iteration of draft, main changes are:

  • revision is the chosen name to talk about the commit identifier. So there is no more RevId, revisionId, etc, and everything should be normalized to revision (or rev when abbreviated in code, but never in serialized form) ;
  • now revision is a mandatory part of a DirectiveId which is now defined as:
final case class DirectiveId(uid: DirectiveUid, rev: Revision = GitVersion.defaultRev) { ... }

The old "DirectiveId" type is renamed into DirectiveUid (for "unique identifier") and the corresponding uid attribute name is used in the new DirectiveId to refer to it.
uid was prefed to uuid because 1/ that allows to use that name if needed even for system rules/directives for some have an human readable name (system ones), and 2/ perhaps we will want to change from uuid to something else latter on.

Below, the updated list of main changes:

  • add a revision for directive and allow to specify it in rule: lots of files change, but it's because we change directive identification fromDirectiveId to DirectiveUid + Revision
  • load a directive version given an URL in directive page: deals with web part of querying a past directive and display it in UI
  • allows to add/remove directives from fullActiveTechniqueLib: this one is the main change in generation logic: when we look for all directives at start of generation, we also fetch directive with past revision when needed (and we clean-up the resulting structure to have less things in memory). In the long term, this will be replace by just having a pair of config revision+facts revision and lookup in git repositories all info ;
  • some change to TechniqueVersion to add Revision to it, and corresponding logic to fetch technique matching directive id (by default or revision specified in directive)

See also internal documentation (development notes and updated architecture in https://www.notion.so/Versionning-des-objets-de-configuration-0d29f58ac58a42d38551a9fe118fe713)

@fanf fanf requested a review from ncharles October 7, 2020 15:23
@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch 3 times, most recently from c8a6137 to f39f6f6 Compare October 7, 2020 15:57
@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from f39f6f6 to 9a2e475 Compare December 3, 2020 00:40
@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from 9a2e475 to 6b1da3f Compare January 25, 2021 14:37
@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from 6b1da3f to ad00f1b Compare February 16, 2021 19:43
@fanf fanf requested a review from VinceMacBuche March 15, 2021 10:50
@VinceMacBuche
Copy link
Member

If the toString changes and the zio-json would be in a different PR that would have been easier to read !!

@VinceMacBuche
Copy link
Member

I reviewed the PR, apart some naming issues / organisation with id and revId this is ok to me

Only thing is why did we have zio-json in this PR and also it would have been easier to read with the toString changes in another PR

@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from ad00f1b to 17cfed2 Compare May 17, 2021 19:07
VinceMacBuche
VinceMacBuche reviewed Jul 23, 2021
View reviewed changes
...urces/rudder/rudder-core/src/main/scala/com/normation/rudder/domain/policies/Directive.scala
* For backward compatibility, the UID field is named "id" in most serialized format. We will keep
* "uid" in code to avoid code looking like `directive.id.id`.
*/
final case class DirectiveId(uid: DirectiveUid, rev: Revision = GitVersion.defaultRev) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with default value

VinceMacBuche
VinceMacBuche reviewed Jul 23, 2021
View reviewed changes
...r/rudder-core/src/main/scala/com/normation/rudder/repository/xml/GitParseRudderObjects.scala Outdated
*/
def getDirective(id: DirectiveId, revId: RevId): IOResult[Option[(ActiveTechnique, Directive)]] = {
def getDirective(uid: DirectiveUid, rev: Revision): IOResult[Option[(ActiveTechnique, Directive)]] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a DirectiveId which regroup both ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to emphasis the fact that you look for a specific revision explictly here. I latter change the named to getDirectiveRevision for that

VinceMacBuche
VinceMacBuche reviewed Jul 23, 2021
View reviewed changes
...der-core/src/main/scala/com/normation/rudder/services/marshalling/XmlSerialisationImpl.scala
@@ -197,7 +198,7 @@ class DirectiveSerialisationImpl(xmlVersion:String) extends DirectiveSerialisati
, directive : Directive
) = {
createTrimedElem(XML_TAG_DIRECTIVE, xmlVersion) (
<id>{directive.id.value}</id>
<id>{directive.id.uid.value}</id>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we store the revision here ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, by definition, it will be given by the git commit

@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from f07878e to 0a48655 Compare July 23, 2021 15:18
VinceMacBuche
VinceMacBuche reviewed Jul 23, 2021
View reviewed changes
.../sources/rudder/rudder-rest/src/main/scala/com/normation/rudder/rest/lift/DirectiveApi.scala Outdated
@@ -592,6 +598,22 @@ class DirectiveApiService14 (
}
}

def getTechniqueWithVerion(optTechniqueName: Option[TechniqueName], opTechniqueVersion: Option[TechniqueVersion], revision: Option[Revision]): IOResult[Technique] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in function Name (Verion)

@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch 4 times, most recently from 873354b to 7b3b62f Compare July 23, 2021 17:20
@fanf
Copy link
Member Author

fanf commented Jul 26, 2021

PR rebased

@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from 7b3b62f to 262c7c3 Compare July 26, 2021 19:02
@Normation-Quality-Assistant
Copy link
Contributor

This PR is not mergeable to upper versions.
Since it is "Ready for merge" you must merge it by yourself using the following command:
rudder-dev merge https://github.com/Normation/rudder/pull/3247
-- Your faithful QA
Kant merge: "Thoughts without content are empty, intuitions without concepts are blind."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/41907/console)

@fanf fanf force-pushed the arch_18302/make_possible_to_give_a_revision_for_directives_in_rule_and_get_correct_technique_files branch from 262c7c3 to 60af039 Compare July 26, 2021 19:51
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit de1eeaa into Normation:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ncharles ncharles ncharles left review comments

@VinceMacBuche VinceMacBuche VinceMacBuche approved these changes

Assignees
No one assigned
Labels
Ready for merge Trigger test
Projects
None yet
Milestone
No milestone
4 participants
@fanf @VinceMacBuche @Normation-Quality-Assistant @ncharles