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 #18177: Method shared folder should not state that you can use md5 as hash method #1239
Conversation
PR updated with a new commit |
# | ||
# @parameter hash_type Hash algorithm used to check if file is updated (sha256, sha512). Only used on Windows, ignored on Unix. default is sha256 | ||
# # keep md5 and sha1 to support old generic methods, also add a default so you don't have to enter a value on linux, md5/sha1 should go to default | ||
# @parameter_constraint hash_type "select" : [ "", "sha256", "sha512", "md5", "sha1" ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still be displayed in the technique editor under the field as field info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope parameter description is limited to one line contrary to documentation
# @parameter hash_type Hash algorithm used to check if file is updated (md5, sha1, sha256). Only used on Windows, ignored on Unix. | ||
# @parameter_constraint hash_type "select" : [ "md5", "sha1", "sha256" ] | ||
# | ||
# @parameter hash_type Hash algorithm used to check if file is updated (sha256, sha512). Only used on Windows, ignored on Unix. default is sha256 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't have a default here because it would break windows method and we can't force windows plugin version sync easily
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(already thought about adding one as it really inconvenient on Linux, should have added a comment here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can update it, we know all users of the plugins and can make them upgrade, it's so much a PITA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then you need to actually define the default value in the CFengine code with a defaults promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need it in cfengine, and it can be empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value will still be defined but empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, we don't use it with CFEngine
OK, squash merging this PR |
…md5 as hash method
3534e9e
to
7c55cd9
Compare
https://issues.rudder.io/issues/18177