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

Forbid de-registration for on-demand Public Cloud instances #431

Merged
merged 1 commit into from Nov 13, 2019

Conversation

ikapelyukhin
Copy link
Contributor

De-registration removes product release packages, which prevents automatic re-registration of modules. registercloudguest should be used on on-demand instances instead.

@ikapelyukhin ikapelyukhin force-pushed the forbid-deregistration-on-payg-instances branch from aad5385 to 350f805 Compare November 5, 2019 12:41
@ikapelyukhin ikapelyukhin force-pushed the forbid-deregistration-on-payg-instances branch from 350f805 to 6f26802 Compare November 5, 2019 12:44
@suse-tests-pass
Copy link
Collaborator

Well Done! Your tests are still passing.
https://ci.suse.de/job/SCC-Connect-Prophet/255889/console
If the given link has expired,you can force a Prophet rerun by just deleting this comment. (Merged 6f26802 into babb4d0)

if File.exist?('/usr/sbin/registercloudguest')
raise UnsupportedOperation,
'De-registration is disabled for on-demand instances. Use `registercloudguest --clean` instead.'
end
Copy link
Member

Choose a reason for hiding this comment

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

Did you check how deregistration with yast will fail in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@digitaltom I don't think Yast has de-registration functionality at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Hmm that message assumes that users have at least cloud-regionsrv-client 9.0.6. I suppose it is fair to assume that someone that picks up the new version of SUSEConnect also updated the cloud-regionsrv-client package. I am OK making the connection, just wanted to note that it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjschwei yep, I though about this as well, but the alternative to --clean would be manual clean up and I'm not sure how explain that in a short error message.

But as long as modules aren't removed, I guess this should be fine.

if File.exist?('/usr/sbin/registercloudguest')
raise UnsupportedOperation,
'De-registration is disabled for on-demand instances. Use `registercloudguest --clean` instead.'
end
Copy link
Member

Choose a reason for hiding this comment

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

Hmm that message assumes that users have at least cloud-regionsrv-client 9.0.6. I suppose it is fair to assume that someone that picks up the new version of SUSEConnect also updated the cloud-regionsrv-client package. I am OK making the connection, just wanted to note that it exists.


context 'when running on on-demand instance' do
before do
allow(File).to receive(:exist?).and_call_original
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be treated as an expect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on your view on whether expectations should be allowed in before block. Rubocop complains about that, though.

Copy link
Contributor

@josegomezr josegomezr left a comment

Choose a reason for hiding this comment

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

If all the other comments are addressed, then lgtm.

@ikapelyukhin ikapelyukhin merged commit 5f89dec into master Nov 13, 2019
@ikapelyukhin ikapelyukhin deleted the forbid-deregistration-on-payg-instances branch November 13, 2019 13:06
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.

None yet

5 participants