Encourage the use of root_path in production to ensure single deployment #1712
Conversation
| // Just setting run_as is not enough to guarantee a single deployment, | ||
| // and neither is setting a principal. | ||
| // We only show a warning for these cases since we didn't historically | ||
| // report an error for them. |
There was a problem hiding this comment.
So this is breaking IFF:
- You're a regular user
- You don't have
run_asset (i.e. it runs as self)
?
There was a problem hiding this comment.
We previously showed an error under those conditions ('run_as' must be set). We now show an error about root_path.
There was a problem hiding this comment.
Do you think we should continue to comment setting run_as, even if root_path is configured?
If a user goes in and looks at defaults and configures it to ~/some/path, it'll still have multiple deployments if multiple people deploy it, even though the warning is silenced.
There was a problem hiding this comment.
I do think it's a good practice to set run_as despite its limitations and it seems okay to set it in the templates. Providing a warning when you only set run_as and don't set root_path seems like a sweet spot to me. Providing an error would be a breaking change and doesn't really seem warranted.
There was a problem hiding this comment.
Do you think we should de-emphasize run_as in general?
There was a problem hiding this comment.
run_as still seems helpful to me and it will become much more usable once we add support for it in pipelines.
Co-authored-by: Pieter Noordhuis <[email protected]>
| // Just setting run_as is not enough to guarantee a single deployment, | ||
| // and neither is setting a principal. | ||
| // We only show a warning for these cases since we didn't historically | ||
| // report an error for them. |
There was a problem hiding this comment.
Do you think we should continue to comment setting run_as, even if root_path is configured?
If a user goes in and looks at defaults and configures it to ~/some/path, it'll still have multiple deployments if multiple people deploy it, even though the warning is silenced.
| // Clear targets after loading. | ||
| b.Config.Targets = nil | ||
| b.Config.Environments = nil | ||
|
|
There was a problem hiding this comment.
Instead of keeping these around here, could you break out a field on the bundle struct where we can keep a snapshot of the selected target? Then you can interrogate it and there's no risk of other mutators changing it after selection. The targets in the configuration have no significance beyond this point.
E.g.
// Target stores a snapshot of the target configuration when it was selected.
Target *config.TargetThere was a problem hiding this comment.
Isn't it cleaner to just remove the side effect from select_target? Instead of just recording which target is selected, the mutator removes fields, which is a bit hard to discover and not really motivated in the code. It's a bit surprising if you want to build a new mutator that consumes this value. Based on your comments, the motivation is to clean things up in order for consumption by summary/validate; shouldn't we just make that a separate step?
There was a problem hiding this comment.
What I see as a risk is that keeping them around means another location that new mutators can go and look at, even though everything under targets no longer has any effect. Variable interpolation won't run either, so values under it shouldn't be used.
I see how this is most convenient though. @andrewnester What do you think?
There was a problem hiding this comment.
It's just that over the past year I ran into this problem twice. You need to use a step through debugger to find where this property is secretly deleted. And the code that deletes it includes no rationale and is just meant to select the default target.
There was a problem hiding this comment.
Alright, I don't want to leave this PR open just because we can't make a decision here. I added 6ea5306 which merges the cleanup behavior back into SelectTarget and adds a few comments about the behavior for maintainers.
lennartkats-db
force-pushed
the
cp-encourage-root-path
branch
2 times, most recently
from
742210c to
3973c50
Compare
lennartkats-db
force-pushed
the
cp-encourage-root-path
branch
from
3973c50 to
6ea5306
Compare
|
@pietern @andrewnester could you take another look at this PR? The remaining thread should be resolved: #1712 (comment) |
bundle/config/bundle.go
Outdated
| @@ -18,6 +18,9 @@ type Bundle struct { | |||
| // Target is set by the mutator that selects the target. | |||
| Target string `json:"target,omitempty" bundle:"readonly"` | |||
|
|
|||
| // TargetConfig stores a snapshot of the target configuration when it was selected by SelectTarget. | |||
| TargetConfig *Target `json:"target_config,omitempty" bundle:"internal"` | |||
|
|
|||
There was a problem hiding this comment.
If located here, it is a part of the configuration, therefore the schema, the output of validate, etc.
Please move this to be a property on the bundle.Bundle type to avoid this.
There was a problem hiding this comment.
👍 Ah, yes, you're right. I moved it.
Do you still think it's the right balance to have that extra property there. It's an extra field that copies the bundle config but it only has a value depending on which state the deployment is in. I'm okay with the proposed approach but it seems cleaner to me to have fewer properties and to do a cleanup step just before summary/validate.
|
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
lennartkats-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Changes
This updates
mode: productionto allowroot_pathto indicate uniqueness. Historically, we requiredrun_asfor this, which isn't actually very effective for that purpose.run_asalso had the problem that it doesn't work for pipelines.This is a cherry-pick from #1387