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

Yomi changes for chroot module #164

Closed
wants to merge 3 commits into from

Conversation

aplanas
Copy link
Contributor

@aplanas aplanas commented Jul 24, 2019

What does this PR do?

Fix a bug during the call of the kwargs parameters for chroot.call()

Tests written?

Yes

(backport saltstack/salt#53996)

(cherry picked from commit cdf74426bcad4e8bf329bf604c77ea83bfca8b2c)
(cherry picked from commit 7f68b65b1b0f9eec2a6b07b02714ead0121f0e4b)
(cherry picked from commit 39da1c69ea2781bed6e9d8e6879b70d65fa5a5b0)
@meaksh
Copy link
Member

meaksh commented Jul 24, 2019

@aplanas, it would be really nice if you could group all these PRs related with yomi/chroot all together in one PR. That way, we will "squash & merge" that PR and produce only one single patch for our Salt package instead of multiple ones.

@aplanas
Copy link
Contributor Author

aplanas commented Jul 24, 2019

@meaksh sure sorry, I did that in this way for two reasons:

  • In this way the changes are smaller, easy to review and can be merged independently, no risk of a big patch that is accumulating changes and never gets merged

  • In the last meeting we agree on splitting the changes, and was notified that was expected that in the future all my commits correspond to a single change in upstream

I do not mind what policy to follow, as my goal is to make easy for reviews and validate, but I would expect some agreement that do not change over the time. Remember that the last commit was required as a single change, later as a multiple changes, and finally you merged all them together.

@meaksh
Copy link
Member

meaksh commented Jul 24, 2019

Let's clarify it better, maybe we didn't explain the process quite well 😄

  • Every commit on openSUSE-2019.2.0 branch produces a patch on our Salt package when the PR is "squashed & merged".
  • We usually don't want very big PRs, since it's better to have smaller pieces and do not accumulate too much stuff for being merged in a single PR. Especially when the PRs are not aligned with upstream, in order to make the review process faster and smoother.
  • We usually like to have "patches" aggregated by topic, even it that involves multiple upstream PRs. So the main review process is made normally upstream, and we just simply double check. This allows us to have less patches (currently we already have 70 patches on top of 2019.2.0 release!) and have them grouped by topic.

So, in this particular case, it seems to me that you're working on fixing some issues all related with yomi, so of course, as much as you can, it would be nice to have all those fixes in a single PR.

Sometimes, of course, this is not possible since there are unexpected changes. That's perfectly fine.

I hope this clarifies a bit more want we all want 😉

@aplanas aplanas changed the title Yomi chroot Yomi changes for chroot module Jul 24, 2019
@aplanas
Copy link
Contributor Author

aplanas commented Jul 24, 2019

So, in this particular case, it seems to me that you're working on fixing some issues all related with yomi, so of course, as much as you can, it would be nice to have all those fixes in a single PR.

You are right, but as we learn in the big patch experience, Yomi is quite broad. A lot of modules, states and event part of the core are touched under this topic. Not counting bugs found the normal process of using Salt. For example, in the already merged commit there are quite number of fixes outside of the Yomi topic.

Another way of viewing those changes are like they are in upstream: split by module or by feature. The changes for this PR are unrelated to the other one and the next one to come, under this criteria.

I do not mind to close this one and merge all together, but in this case good are the chances that we will make the same mistakes that we did previously: accumulate tons of code in an every day a bigger patch.

@aplanas aplanas closed this Jul 24, 2019
@aplanas aplanas deleted the yomi_chroot branch July 24, 2019 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants