Skip to content
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

add redirect uris #30

Closed
wants to merge 11 commits into from
Closed

add redirect uris #30

wants to merge 11 commits into from

Conversation

paulbdavis
Copy link
Contributor

Adds redirect URIs to the CRD

Copy link
Contributor

@jakkab jakkab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks super solid, thanks @paulbdavis! Please consider some minor improvements, as listed below. Would you mind updating the CR syntax section in the ory/k8s repo?https://github.com/ory/k8s/blob/master/docs/helm/hydra-maester.md

@paulbdavis paulbdavis mentioned this pull request Nov 6, 2019
5 tasks
@paulbdavis
Copy link
Contributor Author

Should the version of the API for the CRD be bumped?

The issue is that if you are (like I am) running multiple versions of the maester in a k8s cluster in different namespaces (with multiple instances of hydra itself as well) you have to ensure they are all using the same version if the CRD changes.

If the api version is bumped for the CRD, then multiple versions can co-exist (for testing, staggered upgrades, etc)

@paulbdavis
Copy link
Contributor Author

I added a branch with this change that I can add to this PR here.

@jakkab
Copy link
Contributor

jakkab commented Nov 12, 2019

Should the version of the API for the CRD be bumped?

@paulbdavis No, I don't think so. @piotrmsc how 'bout you?

@piotrmsc
Copy link
Collaborator

piotrmsc commented Nov 12, 2019

@jakkab IMO there's no need, helm upgrade should work as well as kubectl apply -f crd.yaml

@jakkab
Copy link
Contributor

jakkab commented Nov 12, 2019

@paulbdavis if you need help with the CI, take a look at the configuration file here: ory/oathkeeper-maester@c1bcd3d. Instead of go install, we just use a bare binary file downloaded using curl.

@paulbdavis
Copy link
Contributor Author

@piotrmsc Yeah I think you're right. All of the added fields here and in #35 are optional, so it wouldn't break existing resources.

I'll update #35 to change it back to v1alpha1

@aeneasr
Copy link
Member

aeneasr commented Nov 12, 2019

I've restarted the CI which failed with a network error I believe.

@paulbdavis
Copy link
Contributor Author

@aeneasr it was failing because the module path for kustomize changed, it's fixed in #35

@aeneasr
Copy link
Member

aeneasr commented Nov 14, 2019

Merged with #35

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants