Skip to content
This repository was archived by the owner on Dec 7, 2022. It is now read-only.
/ pulp Public archive

1080647 - added validation that a unit profile is not None before reques...#890

Merged
skarmark merged 3 commits intomasterfrom
skarmark-1080647
Apr 11, 2014
Merged

1080647 - added validation that a unit profile is not None before reques...#890
skarmark merged 3 commits intomasterfrom
skarmark-1080647

Conversation

@skarmark
Copy link
Contributor

@skarmark skarmark commented Apr 9, 2014

...ting applicability regeneration by repos

https://bugzilla.redhat.com/show_bug.cgi?id=1080647

…uesting applicability regeneration by repos
@bowlofeggs bowlofeggs self-assigned this Apr 9, 2014
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment isn't quite accurate. The problem is that the consumer's profile changed, and there was an existing RepoProfileApplicability object that referenced the consumer's old profile that hasn't been cleaned up yet. We have a monthly task that looks for stale objects and cleans them out, and until that happens there can be dangling references.

I believe it is safe to remove these dangling RepoProfileApplicability objects here, since they reference profiles that no longer exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

That said, I also do not believe it is harmful to leave the dangling objects in place, and rely on the cleanup to take care of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the unit test you've written does not actually trigger this condition, and this line is not tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment.

@bowlofeggs
Copy link
Contributor

I think this is the correct fix for the bug, but the unit test doesn't create the condition for the bug and therefore doesn't assert that the flaw is fixed. Also, we should fix the comment in the code to be accurate.

Let's make sure we have coverage on the fix and strong assertions before we merge this.

@skarmark
Copy link
Contributor Author

Please re-review for updated comment and unit test.

Copy link
Contributor

Choose a reason for hiding this comment

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

s/In case/In the case/

@bowlofeggs
Copy link
Contributor

Merge when you are ready.

skarmark pushed a commit that referenced this pull request Apr 11, 2014
1080647 - added validation that a unit profile is not None before requesting applicability regeneration by repos
@skarmark skarmark merged commit 1f24760 into master Apr 11, 2014
@skarmark skarmark deleted the skarmark-1080647 branch April 11, 2014 18:51
tehsmyers pushed a commit to tehsmyers/pulp that referenced this pull request Feb 11, 2016
The implementation was using a depth-first-search which did not
traverse in the same order on all platforms. Some Puppet modules
contain other modules as dependencies, and the DFS implementation
could incorrectly find the wrong one. This uses a very dumb
approach which looks for metadata.json in the top level folder
of the the tar.gz uploaded Puppet module. This is based on the
Puppet packaging guidelines [0].

[0]:  https://docs.puppetlabs.com/puppet/latest/reference/modules_fundamentals.html#module-fundamentals

https://pulp.plan.io/issues/890
closes pulp#890
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants