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

#156 - Added skip_types and optimize options to rpm_sync #158

Merged
merged 1 commit into from
Apr 16, 2024

Conversation

chtaylo2
Copy link
Contributor

@chtaylo2 chtaylo2 commented Apr 12, 2024

#156

Adding support for "skip_types" and "optimize" options to rpm_sync. Created tests inside "rpm_sync.yaml". Finally, refreshed the fixtures to reference the new tests.

I've run the full set of tests and all are passing, 100%.

Thanks,
Chris

@chtaylo2 chtaylo2 changed the title #157 - Added skip_types and optimize options to rpm_sync #156 - Added skip_types and optimize options to rpm_sync Apr 12, 2024
@@ -106,6 +126,18 @@ def main():
mirror = module.params["sync_policy"] == "mirror_complete"
parameters = {"mirror": mirror}

parameters = {"optimize": module.params["optimize"]}
Copy link
Member

Choose a reason for hiding this comment

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

you are overwriting what we already collected in parameters here. This cannot possibly be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call out, I'll fix that. I also realized there's no test for the sync_policy key:pair. I'll proactively add that in.

Copy link
Member

Choose a reason for hiding this comment

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

I guess you can fold that into the same update calls, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, exactly. I'll do that

@mdellweg
Copy link
Member

Please add fixes #156 to the commit message

@mdellweg
Copy link
Member

Can you please squash the commits?

Did you know that running make format should help you getting the formatting right? (OK, it's missing from make help...)

@chtaylo2
Copy link
Contributor Author

Rebase pushed to squash previous commits

@mdellweg
Copy link
Member

Looks good now. But we need some engineering on the commit message.
Can you do git commit --amend and change that to

Add skip_types and optimize options to rpm_sync

fixes #156

@chtaylo2
Copy link
Contributor Author

chtaylo2 commented Apr 15, 2024

Please add fixes #156 to the commit message

Got it. I misread your request above to add "more" fix details to the commit message. Glad to see you want this limited and details to remain in the issue tracker. Commit message changed.

@mdellweg mdellweg linked an issue Apr 16, 2024 that may be closed by this pull request
@mdellweg
Copy link
Member

Please add fixes #156 to the commit message

Got it. I misread your request above to add "more" fix details to the commit message. Glad to see you want this limited and details to remain in the issue tracker. Commit message changed.

Well, i don't want to push too hard. Also there is not much more one can say anyway. But I am a big fan of good commit messages (https://cbea.ms/git-commit/) because that's the only thing really persisting. And i hate how GH UI is trying hard to draw every attention away from commit messages.

@mdellweg mdellweg merged commit 40fb60b into pulp:develop Apr 16, 2024
12 checks passed
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.

Adding skip_types & optimize options to rpm_sync.py
2 participants