Don't hardcode the registry URL to authenticate to, use the API response #1525
Conversation
ashb
requested review from
neel-astro,
kushalmalani,
sunkickr and
vandyliu
as code owners
|
I haven't yet tested this against the Houston API. That needs to happen. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1525 +/- ##
==========================================
+ Coverage 85.96% 86.01% +0.05%
==========================================
Files 112 112
Lines 14962 14961 -1
==========================================
+ Hits 12862 12869 +7
+ Misses 1265 1261 -4
+ Partials 835 831 -4 ☔ View full report in Codecov by Sentry. |
Adding @rujhan-arora-astronomer @rishkarajgi to confirm the software/deploy changes |
|
Ran e2e tests from a binary of your branch, and tests around deploy look good - https://app.circleci.com/pipelines/github/astronomer/astro/115485/workflows/8cfe0817-29a4-4e92-b81b-c05534b7c3bc/jobs/5304060 |
ashb
force-pushed
the
dont-hardcode-registry-url
branch
3 times, most recently
from
214e260
to
9ba49a9
Compare
ashb
force-pushed
the
dont-hardcode-registry-url
branch
from
9ba49a9
to
9cff3a1
Compare
|
I've increased coverage a little bit and refactored to get it a bit higher, but hitting the patch target isn't practical I don't think |
There were a few paths where the registry to log in to was hardcoded based on the environment (but not all of them -- for instance the Push path using the go client library was already using this field to auth against!) This change makes the Push and Pull flow use the pre-existing `ImageRepository` flow in the Deploy(ment) response from Astro. This also removes a bit of duplicated code -- various places were stripping of the `Bearer ` prefix from the password before calling the Push function, but that function itself did that.
ashb
force-pushed
the
dont-hardcode-registry-url
branch
from
9cff3a1
to
0d6ec95
Compare
| } | ||
| return nil | ||
| parts := strings.SplitN(imageName, "/", 2) |
There was a problem hiding this comment.
could we try this change out with a custom registry on Software, not sure if it will break or not @rujhan-arora-astronomer @rishkarajgi
with a custom registry, our image name would be of the form fmt.Sprintf("%s:%s", registry, fmt.Sprintf("%s-%s", name, tag))
There was a problem hiding this comment.
Oh yeah, there's a good chance that this won't work in that case.
There was a problem hiding this comment.
We already have the domain here, so we could look if we are in custom registry mode and return.... registry? in that case?
There was a problem hiding this comment.
If I recollect correctly, the reason we had such a structure was that some of the custom registry endpoints were of the form: custom-registry.com/dev-airflow, and it won't allow us to append /<image_name> after that.
The only part I am uncertain about is whether trying to log in to just custom-registry.com would work or not.
There was a problem hiding this comment.
Wait a sec, for Software we don't pass username, so we won't try to login during image push, so I think we are good 🙂.
For software either we log in to the registry during astro login or we expect the customer to log in manually for the custom registry.
There was a problem hiding this comment.
Oh yeah!
The login has to be to custom-registry.com (i.e. without the repo, but it's not always a bare hostname, it could be internal.company.com/registry for instance)
So we're good to merge this?
Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>
…nse (#1525) There were a few paths where the registry to log in to was hardcoded based on the environment (but not all of them -- for instance the Push path using the go client library was already using this field to auth against!) This change makes the Push and Pull flow use the pre-existing `ImageRepository` flow in the Deploy(ment) response from Astro. This also removes a bit of duplicated code -- various places were stripping of the `Bearer ` prefix from the password before calling the Push function, but that function itself did that. Co-authored-by: Neel Dalsania <neel.dalsania@astronomer.io>
Description
There were a few paths where the registry to log in to was hardcoded
based on the environment (but not all of them -- for instance the Push
path using the go client library was already using this field to auth against!)
This change makes the Push and Pull flow use the pre-existing
ImageRepositoryflow in the Deploy(ment) response from Astro.This also removes a bit of duplicated code -- various places were
stripping of the
Bearerprefix from the password before calling thePush function, but that function itself did that.
🧪 Functional Testing
I have used this to deploy to Astro cloud without trouble
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft