pivotal-cf / kiln

Kiln helps you maintain product tiles for VMware Tanzu Operations Manager.
Apache License 2.0
30 stars 20 forks source link

Support GitHub Enterprise for Kiln Release Notes Cmd #505

Closed crhntr closed 1 month ago

crhntr commented 3 months ago

story: TPCF-26493

  1. Rename github_token to github_access_token. But the name has not been changed in the Kilnfile for a release source as the github_token could point to either github.com or enterprise github.
  2. Scope is limited to the kiln release-notes command for now. It can later be extended to other kiln commands.
  3. Use github_enterprise_access_token if there are bosh releases in enterprise github repository.

Assumption: If there are multiple release sources with the same org but different github access tokens, we will iterate through every release source which has org matching the org in the github_repository link specified for the release in the Kilnfile.

crhntr commented 3 months ago

Some questions we had while implementing this:

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.

pvaramballypivot commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

@crhntr

crhntr commented 2 months ago

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 commented 2 months ago

Meeting scheduled for 10/02

pvaramballypivot commented 1 month ago

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 commented 1 month ago

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

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

jajita commented 1 month ago

The scope of this change is limited to the release-notes cmd for now and can be extended to other commands in the future commits/PR.