-
Notifications
You must be signed in to change notification settings - Fork 19
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
Support GitHub Enterprise for GitHub Release Source #505
base: main
Are you sure you want to change the base?
Conversation
Some questions we had while implementing this:
Known Issue To fix this, we need to change how the GitHub client used for release note generation is configured. Instead of passing the github-token as a flag we need to properly parse release source client for each release. |
4a95629
to
40e8979
Compare
story: TPCF-26493 Co-authored-by: Joe Eltgroth <[email protected]>
40e8979
to
173c1aa
Compare
Co-authored-by: Joe Eltgroth <[email protected]> story: TPCF-26493 this is required for generating tile release notes
33c9c56
to
b2ce0ba
Compare
@crhntr I tested the changes locally and here's what I noticed
|
I had hoped to keep it backwards compatible. Unfortunately, now we need a second configuration value (the GitHub enterprise host). We could add another environment variable for that, but we already have it configured in the Kilnfile. Also with the introduction of a second GitHub host, some BOSH Releases could come from enterprise and others from open source, each host requires a different token and we use the Kilnfile.lock to resolve which credentials to use. For local development, tile authors should have "~/.kiln/credentials.yml" configured so only automation scripts should break. |
|
If the Kilnfile uses variables (artifactory, aws...), they all need to be provided before you can parse it. Before this change, it was providing only one GitHub token from the environment or a flag. We did not use the release_sources section in the Kilnfile at all. With regard to the second issue, I wanted to run kiln against TAS to make sure I didn't break anything. Unfortunately, I was not able to get it to run locally due to the requirement on TrainStat. I can try to dig into the BOSH Release details missing but would prefer to pair with someone on RelEng since there is now a (a strictly RelEng) TrainStat dependency in the release-notes code path. |
Meeting scheduled for 10/02 |
story: TPCF-26493