Implement [setup] configuration for backends
#355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Integralist
force-pushed
the
integralist/setup
branch
4 times, most recently
from
355c696
to
39f720e
Compare
Integralist
requested review from
a team and
triblondon
and removed request for
a team
triblondon
suggested changes
Aug 3, 2021
There was a problem hiding this comment.
Summary of discusison with @Integralist on DM:
- Remove --backend* flags, because this mode of use is compatible with creating exactly one backend, and that doesn't really make sense anymore.
- Add an --accept-defaults flag, which performs the deploy non-interactively. That means if the project doesn't have a [setup], the service is created with an 'originless' backend (😂). If the project does have [setup], we use it to create the backends specified, which provides a route for CI-based deploys with specific service config requriements.
- If the project does not have [setup] and the
--accept-defaultsflag is not specified, prompt the user repeatedly for backend info until we receive an empty response.
Some additional thoughts:
- Can we please make it clear what we are naming the backends when we create them, so that the user knows how to reference them in code? And ideally name them something that doesn't look like a hostname. If I specify 'example.com' as my backend host, how about
backend_example_comas the name? If the backend name looks like a hostname, the user might well mistakenly think they can just change the value in code to target a different server. - Confirming also that backends should now be created with host_header property set automatically. Is that changing in this PR?
- The PR description notes a blocking relationship between this PR and Provide
[setup]configuration compute-starter-kit-rust-default#42. is that because the CLI will remove a [setup] section currently? If the existence of [setup] is inert until this PR ships, maybe these PRs don't need to block one another?
Integralist
force-pushed
the
integralist/setup
branch
from
361f427
to
8b91613
Compare
Integralist
force-pushed
the
integralist/setup
branch
from
3ca6167
to
f30975a
Compare
Manual testing notesBelow is the output from some manual testing I carried out for both scenarios where there was New
|
triblondon
approved these changes
Aug 9, 2021
There was a problem hiding this comment.
- "Origin" => "Backend" to be consistent
accept-defaultsshould also accept the default for the domain- Remove 'originless' from the default value for non-setup backend address
- Backend provisioning output verbose by default and make it `Creating backend {name} (host: {host}, port: {port})
- Suppress output for creation of originless backend
- Where a backend doesn't have a name defined, generate
backend_{idx}instead of something based on the hostname or IP (except for the 'originless' special case)
Manual testing notes (part 2)Below is the output from some manual testing I carried out for both scenarios where there was New
|
Integralist
added a commit
that referenced
this pull request
Aug 10, 2021
* add new setup field * refactor deploy logic to handle backends via [setup] configuration * implement test for new backends setup config * fix old backend error test to display address * Some fixes to the logic found during manual testing * add error scenarios to tests * fix issue with publish flags not aligning with deploy * fix deploy tests after rebase * add more [setup] test scenarios * Fix tests to accept multiple mocked prompts * fix logic that validates an existing service * delete any unused mock apis + fix last test to validate another scenario * add new test to validate use of --accept-defaults for existing service * add spacing for messaging * move variable nearer to its use * reduce duplication of originless creation * default to originless creation * add test to validate only some backends are prompted if missing in [setup] scenario * ensure backend name is displayed * Update accept-defaults description and ensure domain isn't prompted * remove references to originless, and use consistent language for backends * ensure originless output is hidden * use index for generating backend names
Integralist
added a commit
that referenced
this pull request
Aug 10, 2021
* add new setup field * refactor deploy logic to handle backends via [setup] configuration * implement test for new backends setup config * fix old backend error test to display address * Some fixes to the logic found during manual testing * add error scenarios to tests * fix issue with publish flags not aligning with deploy * fix deploy tests after rebase * add more [setup] test scenarios * Fix tests to accept multiple mocked prompts * fix logic that validates an existing service * delete any unused mock apis + fix last test to validate another scenario * add new test to validate use of --accept-defaults for existing service * add spacing for messaging * move variable nearer to its use * reduce duplication of originless creation * default to originless creation * add test to validate only some backends are prompted if missing in [setup] scenario * ensure backend name is displayed * Update accept-defaults description and ensure domain isn't prompted * remove references to originless, and use consistent language for backends * ensure originless output is hidden * use index for generating backend names
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The implementation is based on the
[setup]configuration defined in https://developer.fastly.com/reference/fastly-toml/The PR also removes the existing backend related flags as they only worked for a single backend instance. They're replaced by prompting the user multiple times for backends (or using a user's own
[setup]definition).BREAKING CHANGE?: This PR deletes a set of backend flags, thus breaking existing user expectations, but as we are currently in the
0.x.xrange of releases for the CLI I think we should be OK to merge and release these changes as a new0.x.xrelease rather than a1.0.0because the general consensus around the0.x.xline is that the public API should not be considered stable until1.0.0.Workflow scenarios
[setup]defined with Array ofbackendtables:--accept-defaultsis set.--accept-defaultsis set (will create a single 'originless' backend).