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

Embed app config into compiled CLI binary #312

Merged
merged 11 commits into from
Jun 22, 2021
Merged

Conversation

Integralist
Copy link
Collaborator

@Integralist Integralist commented Jun 18, 2021

Problem: The logic for how to handle the local application config file, as well as how to deal with multiple blocking calls to load the config from a remote endpoint, has become complex and is prone towards hard to debug errors.

Solution: Embed the latest version of the app config into the compiled CLI binary. This means we remove two blocking remote calls, and we can failover to the embedded config whenever there is an issue trying to read the config from the user's local disk.

PLUS, we'll have tests for the config logic now! 🎉

Notes: We'll use a GitHub Action to ensure when cutting a release that the binary has the latest version (at that time) of the configuration file embedded.

Logic flow:

  • Attempt to read the config from the user's local disk.
    • If that fails, i.e. there is no local config, resort to using the embedded config.
  • We then unmarshal the config into an in-memory struct.
    • If that fails, and we were using...
      • The embedded config:
        • Exit the program with a remediation error because we don't expect our embedded config to be invalid toml.
      • The user's local disk config:
        • Prompt the user, requesting confirmation to use the embedded config (this is because we couldn't parse their local config, meaning we won't have access to their User.Token/User.Email and so that data would be lost if we switched to embedded config without asking the user first).
          • If reading the user's input fails, then return an error.
          • If the user wants to continue, then resort to using the embedded config, and if that fails:
            • Exit the program with a remediation error because we don't expect our embedded config to be invalid toml.
          • If the user doesn't want to continue, then return a remediation error that tells them they'll need to manually resolve any configuration syntax errors.
  • If using the embedded config: ensure LastChecked and Version are set to the current time and current CLI version.
  • We then write the in-memory struct back to disk.
    • This is for the scenario where we're relying on the embedded config.
  • If the config looks to be in a legacy format, then we'll transition them to the embedded config format.
  • If the config shows the CLI Version as not matching the CLI version currently running, then we'll transition them to the embedded config format (because it's possible their config might not be compatible with the updated CLI version).
  • If the config is stale, then we'll fetch the latest config version asynchronously from the remote api endpoint.

@Integralist Integralist added the enhancement New feature or request label Jun 18, 2021
@Integralist Integralist requested a review from kailan June 18, 2021 10:53
@Integralist Integralist force-pushed the integralist/embed-config branch 2 times, most recently from bc9deed to 0104d26 Compare June 18, 2021 16:59
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.

Had a Zoom call with @kailan to review and have some things to double check before requesting a re-review.

.github/workflows/pr_test.yml Outdated Show resolved Hide resolved
cmd/fastly/static/config.toml Outdated Show resolved Hide resolved
pkg/config/data.go Show resolved Hide resolved
@Integralist Integralist force-pushed the integralist/embed-config branch 3 times, most recently from 843b9fc to e6c93d9 Compare June 21, 2021 16:13
@Integralist Integralist changed the title Embed application configuration into compiled CLI binary Embed app config into compiled CLI binary Jun 22, 2021
Copy link
Member

@kailan kailan left a comment

Choose a reason for hiding this comment

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

All functioning as expected on my machine! Walked through code and CI process with @Integralist and suggested some changes, which have now been implemented.

Ship it! 🚀

@Integralist Integralist merged commit 0de98d0 into main Jun 22, 2021
@Integralist Integralist deleted the integralist/embed-config branch June 22, 2021 10:05
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.

2 participants