Handle URL params for AptRepoFiles #3454
Conversation
This change allows 'AptRepoFiles' to handle URL parameters for "Components" and "Suites" giving it more flexibility rather than always using a fixed default value.
There was a problem hiding this comment.
Thanks for the updates, although I'm a bit puzzled why the existing PR was closed (especially since the source branches of the old PR and this one are in ATIX fork...); anyway, this applies what I said back in the old PR: I'm switching it to draft, until the changes in katello are actually merged.
| @@ -461,19 +461,28 @@ def sections(self): | |||
|
|
|||
| def fix_content(self, content): | |||
| # Luckily apt ignores all Fields it does not recognize | |||
| baseurl = content["baseurl"] | |||
| parsed_url = urlparse(unquote(content["baseurl"])) | |||
There was a problem hiding this comment.
is unquote() still needed? see the discussion on this in #3223
There was a problem hiding this comment.
At least the Candlepin version available for Katello 4.13 (the latest supported release), currently Candlepin 4.4.14 is still affected. I have spent some time looking, but was not able to find any new information when or if Candlepin intends to go back to the old behavior. Given nothing has happened over several months, I can only assume it is not a major priority. The good news is that using unquote will work equally well for quoted and unquoted URLs. As a result I lean towards leaving it in, at least until there is any new information from Candlepin.
| @@ -461,19 +461,28 @@ def sections(self): | |||
|
|
|||
| def fix_content(self, content): | |||
| # Luckily apt ignores all Fields it does not recognize | |||
| baseurl = content["baseurl"] | |||
| parsed_url = urlparse(unquote(content["baseurl"])) | |||
| baseurl = parsed_url._replace(query="").geturl() | |||
| url_res = re.match(r"^https?://(?P<location>.*)$", baseurl) | |||
There was a problem hiding this comment.
TBH: if now baseurl is parsed as URL, then it'd be a good idea to replace this regexp to extract hostname/path/etc with the same urllib.parse APIs -- for example replacing the scheme to get the katello URL
no need to do it in the existing comment; either as additional commit on top of this, or as future PR/improvement
There was a problem hiding this comment.
I will have a look into trying this out (likely early next week).
| query = parse_qs(parsed_url.query) | ||
| if "rel" in query and "comp" in query: | ||
| suites = query["rel"][0].replace(",", " ") | ||
| components = query["comp"][0].replace(",", " ") | ||
| else: | ||
| suites = "default" | ||
| components = "all" |
There was a problem hiding this comment.
are both rel and comp query arguments supposed to always be together? can it happen that only one (either of them) be part of the URL? I started to read the katello PR to try to understand that, but sadly I don't ruby...
There was a problem hiding this comment.
They are always expected to both be set (or else not be set at all). I went and double checked the Katello side implementation, and have high confidence this expectation is guaranteed to be met. In a nutshell the source of truth for both pieces of information is an API query to Pulp, whenever the URL needs to be updated. The implementation is specifically designed to ensure that each Pulp repo always contains at least one ReleaseComponent record, which ensures at least one list element for each of rel and comp.
| suites = query["rel"][0].replace(",", " ") | ||
| components = query["comp"][0].replace(",", " ") |
There was a problem hiding this comment.
is they are comma-separated lists, please split+join them rather than replace the separator; even if the likelyhood of mistakes here is small, I've bitten by mishappenings of "oh let's play with lists as strings" many times in the past...
There was a problem hiding this comment.
I will have a look into trying this out (likely early next week).
The last time I adopted a previously opened PR, without opening a new PR, this created small frictions in communication (because it was less clear who was talking to whom). There were small annoyances like lack of ability to "resolve threads". So we decided to try re-opening this time around. Probably does not make a major difference. 😉 |
This change allows 'AptRepoFiles' to handle URL parameters for "Components" and "Suites" giving it more flexibility rather than always using a fixed default value. If the URL supplied by Candlepin does not have any such URL parameters, then everything defaults back to the way it was before.
This change is needed to enable the following Katello feature: Katello/katello#11058
That being said, this change is fully backwards compatible and defines an interface that the Katello side PR will need to respect. The Katello PR is dependent on this PR, not the other way around.
This PR replaces the previous draft PR here: #3223
My apologies for the switch, the idea is to have both this and the Katello PR in the same hand, so I can better pursue the completion of both related PRs.