Skip to content

fix: add permission to enable --generate-cmd#3942

Merged
mergify[bot] merged 3 commits intoaws:mainlinefrom
huanjani:generate-cmd
Aug 29, 2022
Merged

fix: add permission to enable --generate-cmd#3942
mergify[bot] merged 3 commits intoaws:mainlinefrom
huanjani:generate-cmd

Conversation

@huanjani
Copy link
Copy Markdown
Contributor

#3095 changed the session for --generate-cmd from default to EnvManagerRole; that role needs the states:DescribeStateMachine permission.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

@huanjani huanjani requested a review from a team as a code owner August 24, 2022 19:14
@huanjani huanjani requested review from Lou1415926 and removed request for a team August 24, 2022 19:14
@Lou1415926
Copy link
Copy Markdown
Contributor

what do you think about adding a version check like this one:

func (o *jobRunOpts) validateEnvCompatible() error {

so that we remind people to upgrade their environment template so that the env manager role has the permission when --generate-cmd is executed against a job?

@huanjani
Copy link
Copy Markdown
Contributor Author

what do you think about adding a version check like this one:

func (o *jobRunOpts) validateEnvCompatible() error {

so that we remind people to upgrade their environment template so that the env manager role has the permission when --generate-cmd is executed against a job?

Should we do this for everyone who runs task run or just those who use the relatively rare --generate-cmd flag? Never hurts to upgrade env version, but also maybe annoying for people just running a quick task?

@Lou1415926
Copy link
Copy Markdown
Contributor

Should we do this for everyone who runs task run or just those who use the relatively rare --generate-cmd flag? Never hurts to upgrade env version, but also maybe annoying for people just running a quick task?

I think just for those who use --generate-cmd! I agree that people that just run a task (and hence don't need the new permissions) shouldn't be forced to upgrade their env.

I'm happy to merge this as is, and have the version check as a follow-up because like you said, it's relatively rarely used :cool-crying

@huanjani
Copy link
Copy Markdown
Contributor Author

Okay, I can add the version check as a follow-up so this can move. Thanks!

@mergify mergify Bot merged commit 8a4e210 into aws:mainline Aug 29, 2022
mergify Bot pushed a commit that referenced this pull request Sep 22, 2022
Follow-up to #3942. Fails earlier if user's env version doesn't support the `--generate-cmd` flag for the `task run` command due to missing permissions.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.
@huanjani huanjani deleted the generate-cmd branch October 25, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants