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

Generic OAuth - Add Name Field #889

Merged

Conversation

dhhuynh2
Copy link
Contributor

@dhhuynh2 dhhuynh2 commented Feb 2, 2023

Description

The name field is a supported field in the config, but is not available in this operator. Thus, it makes sense to make it available for generic OAuth.

Relevant issues/tickets

#875

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • This change requires a documentation update
  • I have added tests that prove my fix is effective or that my feature works
  • I have added a test case that will be used to verify my changes
  • Verified independently on a cluster by reviewer

@maciekm maciekm mentioned this pull request Feb 6, 2023
5 tasks
@dhhuynh2 dhhuynh2 force-pushed the 20230202-add-name-to-genericoauth branch from 97ddf59 to 53c5887 Compare February 6, 2023 14:54
@weisdd
Copy link
Collaborator

weisdd commented Feb 7, 2023

@dhhuynh2 thanks for the PR! Seems like some of the checks are failing, you can fix that by running the commands listed at the end of those checks and pushing updates to your branch.

@dhhuynh2
Copy link
Contributor Author

dhhuynh2 commented Feb 7, 2023

@dhhuynh2 thanks for the PR! Seems like some of the checks are failing, you can fix that by running the commands listed at the end of those checks and pushing updates to your branch.

Sorry about that @weisdd - hadn't run make fmt.

0ddb600

Should be good for another look.

@NissesSenap
Copy link
Collaborator

@dhhuynh2 can you rebase and run make test as well. After that I think the CI should be passing without any issues.

@dhhuynh2 dhhuynh2 force-pushed the 20230202-add-name-to-genericoauth branch from 8b2e347 to 82386e5 Compare February 8, 2023 14:55
@dhhuynh2
Copy link
Contributor Author

dhhuynh2 commented Feb 8, 2023

@dhhuynh2 can you rebase and run make test as well. After that I think the CI should be passing without any issues.

Just did - make test looked fine locally.

...
?   	github.com/grafana-operator/grafana-operator/v4	[no test files]
?   	github.com/grafana-operator/grafana-operator/v4/api	[no test files]
ok  	github.com/grafana-operator/grafana-operator/v4/api/integreatly/v1alpha1	0.444s	coverage: 2.9% of statements
ok  	github.com/grafana-operator/grafana-operator/v4/controllers	0.214s	coverage: [no statements] [no tests to run]
?   	github.com/grafana-operator/grafana-operator/v4/controllers/common	[no test files]
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/config	1.236s	coverage: 36.2% of statements
?   	github.com/grafana-operator/grafana-operator/v4/controllers/constants	[no test files]
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/grafana	0.430s	coverage: 6.0% of statements
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/grafanadashboard	1.296s	coverage: 2.1% of statements
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/grafanadashboardfolder	0.713s	coverage: 4.7% of statements
?   	github.com/grafana-operator/grafana-operator/v4/controllers/grafanadatasource	[no test files]
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/grafananotificationchannel	1.062s	coverage: 21.3% of statements
ok  	github.com/grafana-operator/grafana-operator/v4/controllers/model	0.353s	coverage: 36.0% of statements
?   	github.com/grafana-operator/grafana-operator/v4/internal/k8sutil	[no test files]
?   	github.com/grafana-operator/grafana-operator/v4/version	[no test files]

@dhhuynh2 dhhuynh2 force-pushed the 20230202-add-name-to-genericoauth branch from 82386e5 to 9568b49 Compare February 8, 2023 15:08
@NissesSenap
Copy link
Collaborator

You need to add the changes done in deploy/manifests/latest/crds.yaml as well.
I have now fixed it and pushed the changes.

@dhhuynh2
Copy link
Contributor Author

dhhuynh2 commented Feb 8, 2023

You need to add the changes done in deploy/manifests/latest/crds.yaml as well. I have now fixed it and pushed the changes.

Ah, think that was added with make fmt at some point. Thanks for correcting.

Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

LGTM

@NissesSenap NissesSenap merged commit c1a0984 into grafana:master Feb 8, 2023
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

3 participants