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

Fixes #31630 - Global Registration module #558

Merged
merged 1 commit into from
Feb 18, 2021

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Jan 25, 2021

bin/hammer host-registration generate-command

@stejskalleos stejskalleos changed the title Fixes #31630 - Global Registration module Global Registration module Jan 25, 2021
@ofedoren ofedoren changed the title Global Registration module Fixes #31630 - Global Registration module Feb 2, 2021
Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

LGTM from hammer perspective, but it would be good to have at least one test. Spoilers alert: the test will fail, because after the Foreman PR is merged, the test data should be regenerated for hammer and included (preferably as a separate commit in case of cherry-picking in the future) in the PR. So let's wait until the main PR gets merged.

@stejskalleos stejskalleos changed the title Fixes #31630 - Global Registration module [WIP] Fixes #31630 - Global Registration module Feb 3, 2021

Verified

This commit was signed with the committer’s verified signature. The key has expired.
kylekurz Kyle Kurz
@stejskalleos stejskalleos changed the title [WIP] Fixes #31630 - Global Registration module Fixes #31630 - Global Registration module Feb 17, 2021
@stejskalleos
Copy link
Contributor Author

@ofedoren rebased & updated (with tests)

Copy link
Member

@ofedoren ofedoren left a comment

Choose a reason for hiding this comment

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

Thanks, @stejskalleos. Probably there will be no need to CP this, so it's OK to have new test data in within this commit.

LGTM and works, merging.

@ofedoren ofedoren merged commit 9d67395 into theforeman:master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants