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

Verify successful bind when creating pulp_rpmbind #279

Merged
merged 1 commit into from Oct 7, 2017

Conversation

alexjfisher
Copy link
Contributor

Solves 2 problems

  • pulp-consumer rpm bind returns 0 even if the repo doesn't exist
  • Binding is asynchronous, so we should wait until it completes

Fixes #276

Solves 2 problems
  * pulp-consumer rpm bind returns 0 even if the repo doesn't exist
  * Binding is asynchronous, so we should wait until it completes

Fixes theforeman#276
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Overall I think this looks very good.

@property_hash[:ensure] = :present
end

def destroy
consumer('rpm', 'unbind', '--repo-id', @resource[:name])
@property_hash[:ensure] = :absent
end

def wait_for_bind
tries ||= 10
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be configurable. What's your experience with how long the waiting typically takes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

During my testing, the process was actually very quick. One 250ms wait was always enough for me. (IIRC I had to set the sleep to 0.01 to actually see more than one retry take place). But I figured it might not always be that fast depending on what else the system was doing at the time or because of network congestion etc. Waiting for no more than 2.5 seconds total seemed reasonable? If it doesn't complete by then, there's probably no point waiting any longer??

Copy link
Member

Choose a reason for hiding this comment

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

That's fair and we can always fix it later.

@ekohl ekohl merged commit a955c40 into theforeman:master Oct 7, 2017
@ekohl
Copy link
Member

ekohl commented Oct 7, 2017

Thanks!

cegeka-jenkins pushed a commit to cegeka/puppet-pulp that referenced this pull request Jul 24, 2019
Solves 2 problems
  * pulp-consumer rpm bind returns 0 even if the repo doesn't exist
  * Binding is asynchronous, so we should wait until it completes

Fixes theforeman#276
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

2 participants