Feature add per dp software registry login#1936
Conversation
ianbuss
left a comment
ianbuss
left a comment
There was a problem hiding this comment.
Is there anything different about the login credentials for per DP registries?
we use the same credentials to login to per dataplane registry - the same oauth token will be used to authenticate the registries. |
pgvishnuram
changed the title
pgvishnuram
requested review from
jeremybeard,
karankhanchandani,
kushalmalani and
neel-astro
as code owners
ianbuss
left a comment
ianbuss
left a comment
There was a problem hiding this comment.
Small comment. We should also try to add at least some description to the PR
| // Switch to per deployment registry login | ||
| err = auth.RegistryAuth(houstonClient, os.Stdout) | ||
| if err != nil { | ||
| logger.Debugf("There was an error logging into registry: %s", err.Error()) |
There was a problem hiding this comment.
Why do we not exit here? It's not going to work if we haven't auth'd right?
There was a problem hiding this comment.
os.exit(1) right
There was a problem hiding this comment.
Better to return the error up to the command so it fails there.
There was a problem hiding this comment.
in existing code as well even if the login fails on push it will get into error - thats why we didnt change this, now should we make this to return error with fail even when the auth gets failed
There was a problem hiding this comment.
Returning the err here is the right call. Better to fail earlier if we know it isn't going to work.
Pull Request Test Coverage Report for Build 240ae823-eeb4-481a-969b-2bf9e9acbf12Details
💛 - Coveralls |
|
Tests are failing |
|
coveralls is failing - tests have already passed |
ianbuss
left a comment
ianbuss
left a comment
There was a problem hiding this comment.
Left a nit but otherwise looks good to me.
| mockURLs := []houston.DeploymentURL{ | ||
| {URL: "https://deployments.local.astronomer.io/testDeploymentName/airflow", Type: "airflow"}, | ||
| {URL: "https://deployments.local.astronomer.io/testDeploymentName/flower", Type: "flower"}, | ||
| {URL: "registry.local.astronomer.io", Type: "registry"}, |
There was a problem hiding this comment.
Docker login will work for both registry.local.astronomer.io and https://registry.local.astronomer.io so might be worth adding both as a test case here in case we ever wanted to change the houston response to be a "real" URL.
There was a problem hiding this comment.
https wont work - we are sending only the url here without https
6 participants
Description
This PR adds an enhancement over how we authenticate against our incluster registry for Astro Private Cloud.
Earlier our implementation was to authenticate the registry at the time astro login to facilitate image push to destination airflow deployments which worked well for single tenant cluster.
Now since we have moved over to multi cluster mode each cluster will have its independent registry and should be authenticate upon deployment selection to push the images to its independent registry service.
This PR adds a extra logic to assert and validate against the version and selects the registry login logic based on the versioning.
🎟 Issue(s)
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft