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

fix: accept GitHub prop overrides #1994

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

boonware
Copy link

@boonware boonware commented Nov 22, 2022

Issue

spinnaker/spinnaker#6766

Details

Allow GitHub auth properties to be overridden when using the github OAuth2 provider. This allows users to use the github provider for configuring GitHub Enterprise, rather than having to use provider type other; using provider type other disables other features, see the parent issue for details.

@boonware
Copy link
Author

@dbyron-sf @ovidiupopa07 @diasjorge Could I get an approval for the build workflow, and a review?

@ovidiupopa07
Copy link
Contributor

@dbyron-sf @ovidiupopa07 @diasjorge Could I get an approval for the build workflow, and a review?

Hi @boonware, I do not have permission to approve workflows. But I can help with the review 😃

@boonware
Copy link
Author

@ovidiupopa07 Thank you, much appreciated.

Copy link
Contributor

@ovidiupopa07 ovidiupopa07 left a comment

Choose a reason for hiding this comment

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

Are there any tests that you can add to verify this new behavior?

@@ -200,4 +213,8 @@ public static Provider fromString(String name) {
+ Arrays.toString(Provider.values()));
}
}

private static boolean isSet(String config) {
return config != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can reuse a common library like the one from apache or from the SpringFramework (StringUtils)

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.

2 participants