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.

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

Fixes #18909: Add an "upgrade only" option to the technique packageManagement #1650

Conversation

Fdall
Copy link
Contributor

@Fdall Fdall commented Feb 16, 2021

@Fdall
Copy link
Contributor Author

Fdall commented Feb 16, 2021

this is just an initial commit for reference

@Fdall
Copy link
Contributor Author

Fdall commented Feb 16, 2021

PR updated with a new commit

@@ -45,6 +37,10 @@
<LABEL>Absent</LABEL>
<VALUE>absent</VALUE>
</ITEM>
<ITEM>
<LABEL>Only if already installed</LABEL>
Copy link
Member

Choose a reason for hiding this comment

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

"Upgrade only if already installed" or something like that ?


methods:
pass1::
# Force t oaudit mode to verify if packages are already installed (whatever their version is)
Copy link
Member

Choose a reason for hiding this comment

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

this should be done only if "state_upgrade_only_${index_pkg}"

@@ -121,5 +156,7 @@ bundle agent package_management_&RudderUniqueID& {

"report_${index_pkg}" usebundle => rudder_common_reports_generic_index("packageManagement", "${class_prefix_script[${index_pkg}]}", "${trackingkey[${index_pkg}]}", "Post-modification script", "${package[${index_pkg}]}", "Execution of the post-modification script", "${index_pkg}"),
ifvarclass => "posthook_specified_${index_pkg}.${inner_class_prefix_${index_pkg}}_repaired";

reports:
Copy link
Member

Choose a reason for hiding this comment

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

why the empty report ?

@Fdall
Copy link
Contributor Author

Fdall commented Feb 17, 2021

PR updated with a new commit

Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

some minor comments.
I'll check the code as well on a rudder instance


methods:
pass1::
# Force to audit mode to verify if packages are already installed (whatever their version is)
"remove_dry_run_mode" usebundle => push_dry_run_mode("true");
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 it be "force dry_run_mode" ? even if it's only a comment, it would help


"report_${index_pkg}" usebundle => rudder_common_reports_generic_index("packageManagement", "${inner_class_prefix_${index_pkg}}", "${trackingkey[${index_pkg}]}", "Package", "${package[${index_pkg}]}", "${state_description[${index_pkg}]} of package ${package[${index_pkg}]}${architecture_description[${index_pkg}]}${version_description[${index_pkg}]}", "${index_pkg}");
# Report special case when the state is "upgrade only" and the package was not found on the machine
"report_na_${index_pkg}" usebundle => rudder_common_report("packageManagement", "result_na", "${trackingkey[${index_pkg}]}", "Package", "${package[${index_pkg}]}", "Upgrade only set but the package was not found on the machine."),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"report_na_${index_pkg}" usebundle => rudder_common_report("packageManagement", "result_na", "${trackingkey[${index_pkg}]}", "Package", "${package[${index_pkg}]}", "Upgrade only set but the package was not found on the machine."),
"report_na_${index_pkg}" usebundle => rudder_common_report_index("packageManagement", "result_na", "${trackingkey[${index_pkg}]}", "Package", "${package[${index_pkg}]}", "Upgrade only set for package ${package[${index_pkg}]} but this package was not found on the machine.", "${index_pkg}"),

pass1::
# Force to audit mode to verify if packages are already installed (whatever their version is)
"remove_dry_run_mode" usebundle => push_dry_run_mode("true");
"already_installed_${index_pkg}" usebundle => package_state_options("${package[${index_pkg}]}", "any", "${architecture[${index_pkg}]}", "${manager[${index_pkg}]}", "present", "${manager_final_options[${index_pkg}]}"),
Copy link
Member

Choose a reason for hiding this comment

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

if we have two different technique, one with upgrade only first and the second with install, it will skip the install part and fail

# The pass2 is not strictly necessary but prevent future issues if the behavior of pre-evaluation changes (because the evaluation of vars + classes takes 2 passes)
pass2::
# Package
"package_${index_pkg}" usebundle => package_state_options("${package[${index_pkg}]}", "${version[${index_pkg}]}", "${architecture[${index_pkg}]}", "${manager[${index_pkg}]}", "${ncf_state[${index_pkg}]}", "${manager_final_options[${index_pkg}]}"),
Copy link
Member

@ncharles ncharles Mar 1, 2021

Choose a reason for hiding this comment

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

this is not correctly executed
I have reports:
verbose: P: From parameterized bundle: _log_v3( {"Presence of package tree in any version","htop","package_state_options_htop","package_state_options_htop_any_default_default_present_","@{args}"})

when calling first with upgrade only, then present
inverting the order partially solved the issue - package is installed

** Add options for package managers
-- Nicolas Charles <nicolas.charles@normation.com> Thu Dec 7 17:16:30 2017
* Version 1.2
** Convert to multiversionned technique
Copy link
Member

Choose a reason for hiding this comment

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

It misses a changelog entry, stating that Felix the great has done this version

@Fdall
Copy link
Contributor Author

Fdall commented Mar 2, 2021

PR updated with a new commit

@Fdall
Copy link
Contributor Author

Fdall commented Mar 2, 2021

PR updated with a new commit

@Fdall
Copy link
Contributor Author

Fdall commented Mar 2, 2021

PR updated with a new commit

-- Nicolas Charles <nicolas.charles@normation.com> Thu Dec 7 17:16:30 2017
* Version 1.2
** Convert to multiversionned technique
-- Félix Dallidet <felix.dallidet@rudder.io> Thu Mar 1 10:23:30 2021
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- Félix Dallidet <felix.dallidet@rudder.io> Thu Mar 1 10:23:30 2021
-- Félix Dallidet <felix.dallidet@rudder.io> Mon Mar 1 10:23:30 2021

* Version 1.2
** Convert to multiversionned technique
-- Félix Dallidet <felix.dallidet@rudder.io> Thu Mar 1 10:23:30 2021
* Version 1.2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Version 1.2
* Version 1.3

@Fdall
Copy link
Contributor Author

Fdall commented Mar 2, 2021

PR updated with a new commit

@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-techniques/pull/1650
-- Your faithful QA
Kant merge: "Happiness is not an ideal of reason, but of imagination."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/36010/console)

Copy link
Member

@ncharles ncharles left a comment

Choose a reason for hiding this comment

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

it is missing the change of list of maintaine version

@Fdall
Copy link
Contributor Author

Fdall commented Mar 2, 2021

PR updated with a new commit

@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-techniques/pull/1650
-- Your faithful QA
Kant merge: "All our knowledge begins with the senses, proceeds then to the understanding, and ends with reason. There is nothing higher than reason."
(https://ci.normation.com/jenkins/job/merge-accepted-pr/36024/console)

@Fdall
Copy link
Contributor Author

Fdall commented Mar 3, 2021

OK, squash merging this PR

@Fdall Fdall force-pushed the ust_18909/add_an_upgrade_only_option_to_the_technique_packagemanagement branch from 225abab to d9ac7c3 Compare March 3, 2021 09:44
@Fdall Fdall merged commit d9ac7c3 into Normation:branches/rudder/6.1 Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants