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

feat(managed-delivery): Retrieve Managed Delivery manifests from GitHub #828

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

kzwang
Copy link
Contributor

@kzwang kzwang commented Aug 1, 2020

Add support for retrieving managed delivery manifests from GitHub repos.

Also fixed GitHub deprecated API authentication through query parameters (https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/)

@spinnakerbot
Copy link
Contributor

We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:

  • igor-web/src/main/groovy/com/netflix/spinnaker/igor/scm/github/client/model/GetRepositoryContentResponse.groovy

See our server-side commit conventions here.


/**
* Wrapper class for a collection of GitHub clients
*/
class GitHubMaster extends AbstractScmMaster {
Copy link
Contributor

Choose a reason for hiding this comment

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

since it's in OSS and we don't really know who is using this class, can it be backward-incompatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current GitHubMaster is just a copy of GitHubConfig, that defines the GitHubMaster bean, which is not used, and it didn't implement any methods from AbstractScmMaster, so shouldn't have backward compatibility issues.

@gal-yardeni
Copy link
Contributor

@emjburns @luispollo appreciate if you could take a look whenever you can!

Copy link
Contributor

@luispollo luispollo left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for contributing this!

@luispollo luispollo added the ready to merge Approved and ready for merge label Aug 6, 2020
@mergify mergify bot merged commit 5f71fbf into spinnaker:master Aug 6, 2020
@mergify mergify bot added the auto merged label Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants