-
Notifications
You must be signed in to change notification settings - Fork 463
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
(CAT-1483) - Enhancement of handling of apt::source's repos and release parameters #1138
Conversation
f90dbd8
to
c0e0eb8
Compare
1e9311a
to
978c343
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the problem in detail enough to suggest how to solve this, but I think a different approach needs to be taken.
978c343
to
56967b5
Compare
Thanks @kenyon for your review, the problem statement here is user can put any random values like |
56967b5
to
0d73bef
Compare
0d73bef
to
2fbb2a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you completely misunderstood the problem. I hope my comments help.
In my opinion it would be way easier if puppet would just follow the the sources.list spec and require a release to be given (and perhaps as a convenience, if none is given default to stable
or $facts['os']['distro']['codename']
), whereas repos
is optional and if none are given it just terminates the apt source after the release.
See #1132 - this is the core problem. |
Thanks for your inputs @XSpielinbox. Wanted to reconfirm my understanding : On Please confirm my understanding? |
Ah, ok. Then the documentation is very confusing in my opinion. It states in the REFERENCE.md, that
Yes, I definitely want |
I just tried by patching local code by making both $repos optional with below manifest :
Run agent which got completed with error :
The generated source list file which terminated on release itself and no repos:
On other side with current code without passing with same manifest it generated following source list
which works perfect. |
Yes, this is to be expected, because the components may only be omitted, if the suite is an absolute suite, meaning it ends with |
Then I think the module should handle if the release contain
|
@XSpielinbox Have created temp commit based on my understanding 003c904 |
No, only if
This would also be an option, though I was actually expecting that the user has to take care of that himself, that this makes sense and puppet would just lay the groundwork to enable him to do so easily. That's why it's crucial to treat But to be clear: I don't care, whether it then drops the component automatically or I explicitly have to state that I don't want a component at all. What matters to me is, that I don't have to do some strange workaround like passing an empty component or specifying a component that is actually a comment to be able to produce the absolutely valid sources.list that I need.
Yes, as stated above, you can have as many |
As it is not part of a PR, I cannot review it, but I commented beneath the commit. |
2fbb2a5
to
3373a52
Compare
@XSpielinbox Have pulled commit changes here, ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor thing, other than that, this now looks good to me! 👍
3373a52
to
ce8e484
Compare
LGTM |
Summary
Generating the correct source list based on the $release and $repos.
Related Issues (if any)
#1132
Checklist
puppet apply
)