GoogleCloudPlatform / artifact-registry-yum-plugin

Apache License 2.0
6 stars 13 forks source link

dnf: fix baseurl option lookup #29

Closed dorileo closed 1 year ago

dorileo commented 1 year ago

Don't rely on config's items lookup but use the already processed options present within repo object.

google-oss-prow[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dorileo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/GoogleCloudPlatform/artifact-registry-yum-plugin/blob/main/OWNERS)~~ [dorileo] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
dorileo commented 1 year ago

/hold Holding this one until we have a word from @ericdand et al.

ericdand commented 1 year ago

This will definitely improve the error message, but I suspect there may be cases where the baseurl is legitimately omitted from config, since we've seen this code falter chiefly on Red Hat repos that I believe come preinstalled on certain of our golden images. It may be the case that missing baseurl should be tolerated -- someone more expert than I in how RHEL systems are configured should be consulted about what the right thing to do is here.

dorileo commented 1 year ago

The configured repos will either have baseurl or mirrorlist, for this plugin use case we are aiming for repos configured with baseurl containing the string 'pkg.dev' so skipping those who don't fall under this criteria should be just fine.

dorileo commented 1 year ago

/unhold

ChaitanyaKulkarni28 commented 1 year ago

/lgtm

rbelnap commented 1 year ago

This update broke authorization for our gcp repositories. The check if 'pkg.dev' in repo.baseurl and not self.error: fails in all cases. This looks to be because repo.baseurl is a libdnf.module.VectorString and not a python string? I'm not familiar with the details there. However, if I cast this to a string it works as intended, e.g. if 'pkg.dev' in str(repo.baseurl) and not self.error:

ericdand commented 1 year ago

Thank you for the report. Don't you love duck typing? It works perfectly fine until some fundamental language feature like the "in" keyword decides it wants to insist upon real, static typing.

The fact that these plugins appear to lack tests is concerning. In addition to fixing this PR, we should bring this repo up to Google's (internal) standards for code coverage and quality. I'll open an issue ticket about this.

rpfernandezjr commented 1 year ago

Do you guys control the google-compute-engine rpm repository? would it be possible to continue to host the previous version in the repo? This will allow us to downgrade and pin to the previous version on GCE instances until a fix is released.