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 release process to not use external config #688

Merged
merged 20 commits into from
Oct 21, 2022

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Oct 19, 2022

The CLI configuration file was being pulled down from a Fastly endpoint that handled the construction of the config (this was because an internal Fastly service already had access to all the starter kit data that was injected into the config for the CLI to consume).

This caused issues with the CLI release process as the person cutting the release might not have waited for the internal system to have run its daily cron that updated the configuration data.

To avoid this race condition we've moved all this logic inside of the CLI's own CI process.

@Integralist Integralist added the enhancement New feature or request label Oct 19, 2022
Copy link
Collaborator Author

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

Just some comments for the reviewer...

pkg/config/config.toml Outdated Show resolved Hide resolved
pkg/config/config.toml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
DEVELOP.md Outdated Show resolved Hide resolved
.github/workflows/pr_test.yml Show resolved Hide resolved
.github/workflows/tag_release.yml Show resolved Hide resolved
Copy link
Contributor

@triblondon triblondon left a comment

Choose a reason for hiding this comment

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

When we chatted about this on zoom, the approach we discussed was to list SK repo URLs in a file in the CLI repo, and then pull the title and description of the SKs from the SK repo's fastly.toml file during the CLI build. It seems like the solution here is to bake the title and description data into the CLI repo directly, which duplicates and disconnects it from the SK's canonical data.

My concern is that this creates a maintenance overhead, and an unexpected gotcha if you update the SK and then expect the output from the CLI to change.

These are the repos I think we should offer in the CLI, in this order:

Could we use that approach, and store only the URLs here in the CLI - adding the other data when the CLI is built?

@Integralist Integralist force-pushed the integralist/fix-release-process branch from 4b6dcbb to f793998 Compare October 20, 2022 19:05
@Integralist Integralist force-pushed the integralist/fix-release-process branch from 7cfa830 to 11a7a4c Compare October 20, 2022 19:11
@@ -0,0 +1,45 @@
#!/bin/bash
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@triblondon here's the script for generating the config.

@Integralist Integralist force-pushed the integralist/fix-release-process branch 2 times, most recently from dc2fd1e to 0a24f3b Compare October 20, 2022 19:27
@Integralist Integralist force-pushed the integralist/fix-release-process branch from 0a24f3b to afc7593 Compare October 20, 2022 19:29
@Integralist Integralist requested a review from fgsch October 21, 2022 09:06
DEVELOP.md Outdated Show resolved Hide resolved
Copy link
Member

@fgsch fgsch left a comment

Choose a reason for hiding this comment

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

LGTM, windows CI failure aside :)

@Integralist Integralist force-pushed the integralist/fix-release-process branch from 41ff2e8 to 7e37a9d Compare October 21, 2022 10:46
@Integralist Integralist force-pushed the integralist/fix-release-process branch 22 times, most recently from 403d48e to 1e9fdff Compare October 21, 2022 15:04
@Integralist Integralist force-pushed the integralist/fix-release-process branch from 1e9fdff to 84ff62b Compare October 21, 2022 15:09
@Integralist Integralist merged commit fcd188f into main Oct 21, 2022
@Integralist Integralist deleted the integralist/fix-release-process branch October 21, 2022 17:53
@Integralist Integralist changed the title Fix release process Fix release process to not use external config Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants