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

CFE-2724: Removed quotes from arglist attribute in commands promises #2804

Merged
merged 1 commit into from
Jan 4, 2024

Conversation

larsewi
Copy link
Contributor

@larsewi larsewi commented Dec 14, 2023

Now that whitespaces are preserved in the arglist attribute of the commands promises, we no longer need to use extra quotes. In fact, these quotes become a part of the argument, causing some methods to fail in the MPF.

Merge together with:

Copy link
Member

@nickanderson nickanderson left a comment

Choose a reason for hiding this comment

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

This change as it stands would not work during upgrades while I am running old binaries and new policy.

@@ -757,7 +757,7 @@ bundle agent superhub_schema
"$(cfengine_enterprise_federation:config.bin_dir)/psql_wrapper.sh"
arglist => {
"cfdb",
`"select superhub_schema('$(sys.key_digest)');"`,
"select superhub_schema('$(sys.key_digest)');",
Copy link
Member

Choose a reason for hiding this comment

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

Assuming this change is done for 3.24.0 ... When I upgrade the first thing I will do is upgrade my policy set. So I will be running e.g. 3.18.6 with this policy for some time before I upgrade to 3.24.0.

Ideally we could handle this automatically. Seems like it could be difficult to detect reliably, but this is not an elegant transition.

People will have to duplicate all the cases where they have double quoting in argslist where the behavior changes and make sure that policy uses the right quoting based on the cfengine version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would be a breaking change. Do you think we should not make it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Do I understand correctly? ...

  • Users must locate all use of argslist within their policy
  • Users must add the macro treatment (or similar) as done in this file if the promise using argslist does not execute within a shell.
  • Users must NOT add the macro treatement (or similar) if the promise using argslist uses a shell

Am I overlooking anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds about right :)

Copy link
Member

Choose a reason for hiding this comment

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

Ok, we just need a good change in behavior post to outline the details I guess.

Now that whitespaces are preserved in the arglist attribute of the
commands promises, we no longer need to use extra quotes. In fact, these
quotes become a part of the argument, causing some methods to fail in
the MPF.

Ticket: CFE-2724
Changelog: None
Signed-off-by: Lars Erik Wik <[email protected]>
@larsewi
Copy link
Contributor Author

larsewi commented Dec 20, 2023

Build Status

@larsewi larsewi requested a review from nickanderson December 20, 2023 14:54
@olehermanse olehermanse merged commit b205e36 into cfengine:master Jan 4, 2024
3 of 4 checks passed
@larsewi
Copy link
Contributor Author

larsewi commented Jan 4, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants