-
Notifications
You must be signed in to change notification settings - Fork 330
CLI: Warn if project from flags mismatches project from config #2815
Conversation
7adb147
to
797dbc2
Compare
internal/cli/base.go
Outdated
c.refProject = nil | ||
c.refApps = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little bit messy to undo the refs that were set in the above config-parsing block - if someday we set more refs up there, we'll have to remember to roll them back here.
Moving this flag mismatch check into the config parsing block feels messy too though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like you could move this into the if !baseCfg.NoConfig
block because thats the only place we populate refProject. From there, it feels cleaner to avoid ever setting these extra fields. That way the place where its set is totally just one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, i'm not sure if I entirely follow - we'll still need to set refProject
outside of baseCfg.NoConfig
, because there may be commands that need a project ref but don't want config parsed. I took another stab at it though, moving more logic up into the baseCfg.NoConfig
block to avoid setting the refs in the first place if the config and flag defined project names collide.
Still feels a bit messy, because now we're checking c.flagProject
twice - but it's not too bad. Let me know if you see a better pathway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
internal/cli/base.go
Outdated
c.refProject = nil | ||
c.refApps = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like you could move this into the if !baseCfg.NoConfig
block because thats the only place we populate refProject. From there, it feels cleaner to avoid ever setting these extra fields. That way the place where its set is totally just one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor update request in the warning message, but otherwise looks good to me!
Still feels a bit messy, because now we're checking c.flagProject twice - but it's not too bad. Let me know if you see a better pathway.
Tried to catch up on this conversation....from what you have now I think that seems ok to me. The project flag check is mostly for the warning to be displayed if no config, then the second time we check immediately after is for setting project refs.
d5fa740
to
9479571
Compare
Also force remoteness. If we don't do that, we may spawn a local runner, which will parse the config that we know doesn't match the user's flags.
Co-authored-by: Rae Krantz <[email protected]>
9479571
to
40dcddc
Compare
What changed
A user can add the
-project
flag when they're in some other project's directory:Example:
This means one of two things:
1: They added the flag explicitly, and they want to operate against
project-b
.2: The user wants to operate against the project that matches the directory they're in, but they've copied the
-project
flag from some other command/example.We could just error here, but it feels onerous to force the user from scenario 1 to
cd
into some safe directory before running their command.The middle ground here is to continue to allow flags to override config, but issue a warning to help the user from scenario 2. We also force the operation to happen remotely, as the local runner doesn't currently support cloning down some other project's vcs source.
What this looks like