Skip to content

Allow to pass arguments to fileserver.update runner #55014

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
Merged
merged 4 commits into from
Sep 29, 2020
Merged

Allow to pass arguments to fileserver.update runner #55014

merged 4 commits into from
Sep 29, 2020

Conversation

sathieu
Copy link
Contributor

@sathieu sathieu commented Oct 15, 2019

What does this PR do?

Allow to pass arguments to fileserver.update runner

This is useful to only update one GitFS remote.

What issues does this PR fix or reference?

None. But #53622 is related.

Previous Behavior

No way to only update one GitFS repo.

Tests written?

No

Commits signed with GPG?

No

@sathieu sathieu requested a review from a team as a code owner October 15, 2019 12:46
@ghost ghost requested a review from twangboy October 15, 2019 12:46
@sathieu sathieu force-pushed the fileserver_update_args branch 2 times, most recently from e0f625d to f58e484 Compare October 15, 2019 13:50
@sathieu
Copy link
Contributor Author

sathieu commented Oct 17, 2019

@twangboy @waynew @terminalmage Please review 👓

@dwoz dwoz added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Dec 3, 2019
@sathieu sathieu force-pushed the fileserver_update_args branch 2 times, most recently from 0e99f8e to 70ff03d Compare December 27, 2019 14:26
@sathieu
Copy link
Contributor Author

sathieu commented Dec 27, 2019

@dwoz I've added tests (and rebased).

@sathieu sathieu force-pushed the fileserver_update_args branch from 70ff03d to 17a05a6 Compare December 27, 2019 14:45
@waynew
Copy link
Contributor

waynew commented Feb 28, 2020

@sathieu thanks for the PR and sorry for the delay in getting this reviewed! I've merged the most recent master changes in. Next week we're going to be pretty busy with some planning, but I've written this down to make sure we get this looked at by 2nd week in March (2 weeks from now)

@cmcmarrow cmcmarrow removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Mar 20, 2020
cmcmarrow
cmcmarrow suggested changes Mar 20, 2020
View reviewed changes
Copy link
Contributor

@cmcmarrow cmcmarrow left a comment

Choose a reason for hiding this comment

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

Can we also get test for the changes you made in gitfs?

@sathieu sathieu force-pushed the fileserver_update_args branch from 76b30d1 to f5cebf2 Compare April 4, 2020 13:55
@waynew
Copy link
Contributor

waynew commented Apr 6, 2020

@sathieu I brought this up with the team, and **kwargs is usually an anti-pattern in Salt. Unless there's a really good reason, you should specify the keyword args that you want to be able to use.

With ** it bypasses our args checker, so we can no longer check that states are being used correctly.

If you want to avoid having to provide an argument, default arguments are OK!

Let me know if you have any questions!

@sathieu sathieu force-pushed the fileserver_update_args branch from f5cebf2 to c109964 Compare April 15, 2020 15:16
@sathieu sathieu requested a review from cmcmarrow April 15, 2020 15:17
waynew
waynew suggested changes May 11, 2020
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Could you please update these to no longer take **kwargs?

@sathieu sathieu force-pushed the fileserver_update_args branch 2 times, most recently from 1313fbc to 6366c5d Compare May 14, 2020 19:23
@sathieu sathieu force-pushed the fileserver_update_args branch from 6366c5d to d7695a2 Compare May 28, 2020 15:39
@sathieu
Copy link
Contributor Author

sathieu commented May 28, 2020

I've added a test to ensure that invalid parameters throw an error.

Remaining todo:

Can we also get test for the changes you made in gitfs?

@sathieu sathieu force-pushed the fileserver_update_args branch 3 times, most recently from d990575 to a762f35 Compare May 29, 2020 10:59
@sathieu sathieu requested a review from waynew May 29, 2020 11:00
@sathieu
Copy link
Contributor Author

sathieu commented May 29, 2020

@waynew @dwoz @cmcmarrow @twangboy Please review.

@sathieu sathieu force-pushed the fileserver_update_args branch from a762f35 to 2ba7a12 Compare May 29, 2020 11:40
waynew
waynew previously approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

That seems reasonable to me - I'll bring it up to the rest of the team.

@cmcmarrow
Copy link
Contributor

@sabaini thanks for adding tests. This should get merged shortly after sodium gets released.

@sathieu
Copy link
Contributor Author

sathieu commented Jun 4, 2020

@cmcmarrow I've added the tests 😄

@sathieu sathieu dismissed stale reviews from cmcmarrow and waynew via caaaee1 August 25, 2020 11:29
@sathieu sathieu force-pushed the fileserver_update_args branch from 2ba7a12 to caaaee1 Compare August 25, 2020 11:29
@sathieu
Copy link
Contributor Author

sathieu commented Aug 26, 2020

@cmcmarrow You've marked this PR Merge Ready. I've now rebased and fixed pre-commit and Pylint. Hope you merge it now ...

@sathieu
Copy link
Contributor Author

sathieu commented Sep 3, 2020

@sagetherage You've removed the Merge Ready tag here. What can I do help merging this PR (and others #56316, #57288)?

@sagetherage
Copy link
Contributor

we removed that label all together from the project as it doesn't mean what it says - this needs to be rebased with master, and needs a completed review.

@sathieu
Copy link
Contributor Author

sathieu commented Sep 17, 2020

@sagetherage @cmcmarrow @waynew @twangboy Any chance to get this reviewed and merged soon? It was marked merge-ready few months ago...

Thanks

twangboy
twangboy previously approved these changes Sep 17, 2020
View reviewed changes
@sagetherage
Copy link
Contributor

The team is working on getting the test suite to green overall and then merging will start, again. I haven't updated the branch since the branch builds are failing, so I will get back to this soon.

@sathieu
Copy link
Contributor Author

sathieu commented Sep 29, 2020

@twangboy @sagetherage Hello. Sorry to insist, but can this be merged in magnesium ?

This MR exists since Oct 2019.

NB: I've merged master and resolved the conflict.

@sathieu sathieu force-pushed the fileserver_update_args branch from 6f0a3f9 to 7981e66 Compare September 29, 2020 13:52
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 29, 2020
@dwoz dwoz merged commit 269c12a into saltstack:master Sep 29, 2020
@sathieu sathieu deleted the fileserver_update_args branch September 30, 2020 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwoz dwoz dwoz approved these changes

@waynew waynew Awaiting requested review from waynew

@cmcmarrow cmcmarrow Awaiting requested review from cmcmarrow

@s0undt3ch s0undt3ch Awaiting requested review from s0undt3ch

@twangboy twangboy Awaiting requested review from twangboy

Assignees
No one assigned
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sathieu @waynew @cmcmarrow @sagetherage @s0undt3ch @dwoz @twangboy