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 #7184: Add finer grained generic methods for command execution #234

Closed
wants to merge 1 commit into from
Closed

Fixes #7184: Add finer grained generic methods for command execution #234

wants to merge 1 commit into from

Conversation

peckpeck
Copy link
Member

@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 802bdc7 to 95c0bca Compare September 15, 2015 16:01
@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 95c0bca to d02697b Compare September 16, 2015 09:17
@ncharles
Copy link
Member

could you rebase this PR please ?

@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from d02697b to 871cfd1 Compare September 18, 2015 09:52
ncharles
ncharles reviewed Sep 18, 2015
View reviewed changes
tree/30_generic_methods/command_execution_result.cf
# If an exit code is not in the list it will lead to an error status.
# If you want 0 to be a success you have to list it in the kept_codes list

bundle agent command_execution_result(command, kept_codes, repaired_codes)
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 go also in non-master branch; it would make the life of our users so much better

Copy link
Member Author

Choose a reason for hiding this comment

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

It could

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #7234
The other branch should be merged before this one.

@ncharles
Copy link
Member

I'm ok with the logic; however the comments should be improved
If you speak to somebody that knows CFEngine, he will believe he needs to figure out the value of ${class_prefix}, which is not true - this should be used as it is
The confusion is even more easy to make as class_prefix is used as a result, or a string to fill in, given the case.

One the texts are clarified, you can merge

ncharles
ncharles reviewed Sep 23, 2015
View reviewed changes
tree/30_generic_methods/condition_persistent.cf
# The persistence duration is controlled using ${duration}. There is no way to persist indefinitly.
# To make the condition specific to current technique instance (or directive), use ${class_prefix}_a_name instead of a static name.
#
bundle agent condition(condition_name, duration)
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 the bundle name contain also persistent ?

@ncharles
Copy link
Member

wait - where are the unit tests ???

@peckpeck
Copy link
Member Author

PR updated
test

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 871cfd1 to 433b55d Compare September 28, 2015 15:19
@peckpeck
Copy link
Member Author

PR updated
test2

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 433b55d to 49b2d27 Compare September 28, 2015 15:20
@peckpeck
Copy link
Member Author

PR updated

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 49b2d27 to 9ba5159 Compare September 29, 2015 09:55
@peckpeck
Copy link
Member Author

PR updated
7171

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 9ba5159 to 3609cfc Compare September 29, 2015 09:57
@peckpeck
Copy link
Member Author

PR updated

@peckpeck
Copy link
Member Author

PR updated
Rename class_prefix parameters to avoid cfengine error messages

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 3609cfc to 0f1d214 Compare September 29, 2015 11:46
@peckpeck
Copy link
Member Author

PR updated
Added tests

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 0f1d214 to 1950b34 Compare September 29, 2015 15:03
@ncharles
Copy link
Member

ncharles commented Oct 7, 2015

i don't get it, i merged #245 but it is still referenced in the commits here

@peckpeck
Copy link
Member Author

peckpeck commented Oct 7, 2015

github secrets ...

@peckpeck peckpeck force-pushed the ust_7184/add_finer_grained_generic_methods_for_command_execution branch from 1950b34 to 191eb1e Compare October 7, 2015 16:26
@peckpeck
Copy link
Member Author

peckpeck commented Oct 7, 2015

After a test, github updates the PR only if the original branch was updated, so an amend changed it.
But since the PR was mergeable, I think the commit would have been ignored by github

This was referenced Oct 14, 2015
Merged
Merged
Merged
Merged
Merged
@peckpeck
Copy link
Member Author

Superseded by multiple PRs

@peckpeck peckpeck closed this Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@peckpeck @ncharles