enhance(deploy)!: remove default value for ref in add deployment command #480
Conversation
Codecov Report
@@ Coverage Diff @@
## main #480 +/- ##
=======================================
Coverage 86.37% 86.37%
=======================================
Files 145 145
Lines 6310 6310
=======================================
Hits 5450 5450
Misses 704 704
Partials 156 156
|
ecrupper
changed the title
| }, | ||
| &cli.StringFlag{ | ||
| EnvVars: []string{"VELA_TARGET", "DEPLOYMENT_TARGET"}, | ||
| Name: "target", | ||
| Aliases: []string{"t"}, | ||
| Usage: "provide the name for the target deployment environment", | ||
| Value: "production", |
There was a problem hiding this comment.
Can you help me understand more about this change?
I ask because GitHub's default is production:
https://docs.github.com/en/rest/deployments/deployments?apiVersion=2022-11-28#create-a-deployment
The
environmentparameter allows deployments to be issued to different runtime environments. Teams often have multiple environments for verifying their applications, such asproduction,staging, andqa. This parameter makes it easier to track which environments have requested deployments. The default environment isproduction.
There was a problem hiding this comment.
@ecrupper might chime in too, but it was mainly because we thought "production" was a bit too bold. i'm actually surprised it's the default value on GitHub's end.
there is also a case to be made to let SCM handle the default anyway? what if GitHub changes this on their end?
good news is.. since it's default on GitHub's end, leaving it blank will not impact the result?
as an aside, if we keep this change, we will want to update the UI code as well.
however, if we revert this - i wouldn't mind some comment to explain the choice there though i'd prefer to leave SCM-specific things out since we're trying to be SCM agnostic.
There was a problem hiding this comment.
Yeah I agree that we should just default to whatever the SCM's default is. I'm also surprised it's production.
As long as we're interpreting the target all the same (what we get in the payload), which it looks like we are, then this wouldn't even be breaking, I guess.
Closes go-vela/community#423
Made possible by go-vela/server#961
Also removing the default value of
productionas a deployment target. This seems like it could exacerbate a simple mistake if folks forget to provide a target when intending to target a non-prod environment.Marking this breaking, as potential scripts that depended on those default values (mostly the target, as the ref will be populated with the default branch anyway) will now not work as they used to.