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

Support GitHub Enterprise for GitHub Release Source #505

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

crhntr
Copy link
Contributor

@crhntr crhntr commented Aug 15, 2024

story: TPCF-26493

@crhntr
Copy link
Contributor Author

crhntr commented Aug 15, 2024

Some questions we had while implementing this:

  • Do we need to provide a CA to the http.Client TLS config? Looks like no. We have not tested kiln from CI yet.
  • Do we have API mismatches because our GitHub Enterprise deployment is different from Github.com? Both commands worked with the current go-github version so it seems to work ok.
  • Do we have different api and upload URLs? Nope, it works... also we are not doing uploads in the commands so we are likely not exercising this configuration.

Known Issue
If you have multiple github release sources (ie some releases on GitHub.com and some on GitHub Enterprise), release-notes will fail to generate.

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.

@crhntr crhntr force-pushed the add-github-enterprise-support branch from 4a95629 to 40e8979 Compare August 15, 2024 19:45
story: TPCF-26493

Co-authored-by: Joe Eltgroth <[email protected]>
@crhntr crhntr force-pushed the add-github-enterprise-support branch from 40e8979 to 173c1aa Compare August 15, 2024 19:46
@crhntr crhntr added enhancement tas-slingshots Created by https://github.com/orgs/pivotal-cf/teams/tas-strategic-initiatives-slingshot labels Aug 15, 2024
@crhntr crhntr changed the title Support GitHub Enterprise for GitHub Release Source and Release Note Generation Support GitHub Enterprise for GitHub Release Source Aug 16, 2024
Co-authored-by: Joe Eltgroth <[email protected]>

story: TPCF-26493

this is required for generating tile release notes
@crhntr crhntr force-pushed the add-github-enterprise-support branch from 33c9c56 to b2ce0ba Compare August 16, 2024 23:11
@pvaramballypivot
Copy link
Member

pvaramballypivot commented Sep 10, 2024

@crhntr I tested the changes locally and here's what I noticed

  1. It doesn't seem to be backward compatible. The command now requires artifactory_username, artifactory_password and github_access_token to be passed in the command as variables. Previously just setting the GITHUB_TOKEN env variable was enough. This is not a huge change but it would require updating scripts.
  2. The output is missing the details of the bumps. Check out this story where I have attached 2 files - "current_release_output.md" run from the current released version of kiln, and "pr_changes_output.md" run from the changes in this PR. Please compare the 2 to see more.

@crhntr
Copy link
Contributor Author

crhntr commented Sep 19, 2024

It doesn't seem to be backward compatible. The command now requires artifactory_username, artifactory_password and github_access_token to be passed in the command as variables. Previously just setting the GITHUB_TOKEN env variable was enough. This is not a huge change but it would require updating scripts.

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.

@pvaramballypivot
Copy link
Member

@crhntr

  • The new variables required are the artifactory_username and artifactory_password. Why would we need these with the changes you are trying to make? Shouldn't affect Github enterprise host correct?
  • Did you get a chance to look at my 2nd comment? The payload is in this story - https://jira.eng.vmware.com/browse/TPCF-26865

@crhntr
Copy link
Contributor Author

crhntr commented Sep 26, 2024

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.

@pvaramballypivot
Copy link
Member

Meeting scheduled for 10/02

@jajita jajita force-pushed the add-github-enterprise-support branch from 9a1d40e to 2f28fba Compare October 7, 2024 17:23
Co-Authored-By:  Joe Eltgroth <[email protected]>
Co-Authored-By:  Christopher Hunter <[email protected]>
@jajita jajita force-pushed the add-github-enterprise-support branch from 2f28fba to 852058e Compare October 7, 2024 17:41
@pvaramballypivot
Copy link
Member

I ran this same command without your PR changes and using the current kiln release and it works fine. But when I test with the changes in the PR, I am seeing this error.

../kiln/kiln release-notes -tu https://tas-trainstat.eng.tanzu.broadcom.com -w rc -d 2024-10-05 -k tas/Kilnfile -l tas -l 4.0 -m RT-2024-017 -u ../docs-pas/runtime-rn.html.md.erb --variable artifactory_username="${BCOM_ARTIFACTORY_USERNAME}" --variable artifactory_password="${BCOM_ARTIFACTORY_TOKEN}" --variable github_access_token="${GITHUB_ACCESS_TOKEN}" refs/tags/RT-2024-016-run.5/4.0 refs/tags/RT-2024-017-run.1/4.0
failed to parse owner and repo name from URI "": path missing expected parts

@pvaramballypivot
Copy link
Member

I tested the changes, and here's what I found.

  • If I don't pass the github_token, it throws this error message "could not execute "release-notes": github-token (env: GITHUB_TOKEN) must be set to interact with the github api" which is fine. Once I set the github token, it still throws an error "could not execute "release-notes": template: Kilnfile:12:18: executing "Kilnfile" at <variable "github_access_token">: error calling variable: could not find variable with key "github_access_token"
  • If I pass the "github_access_token" variable and it's empty, it just goes into an infinite loop by throwing this msg "no token passed for github release source".
  • Also, we discussed this before. If it's possible to not ask for artifactory username and password (like how it works currently), that'd be great. 1 because that would mean no script changes for us, and 2 coz these variables are not used in this command, so seems useless.

But after passing all the variables, it worked as expected.

jajita and others added 3 commits October 14, 2024 11:13
use "github_host" env var to fetch release notes for bosh releases bump

Co-authored-by: Nick Rohn <[email protected]>
@jajita jajita force-pushed the add-github-enterprise-support branch 2 times, most recently from 96e337d to 4e3e430 Compare October 18, 2024 00:30
…atch the release sources to the release by org
@jajita jajita force-pushed the add-github-enterprise-support branch from 4e3e430 to 5688b1e Compare October 18, 2024 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready-for-review tas-slingshots Created by https://github.com/orgs/pivotal-cf/teams/tas-strategic-initiatives-slingshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants