refactor: remove aws flag helper message #9080

simar7 · 2025-06-25T23:58:55Z

Description

We've removed aws live cloud scanning from trivy for about a year now. This PR removes the helper message.

Checklist

I've read the guidelines for contributing to this repository.
I've followed the conventions in the PR title.
I've added tests that prove my fix is effective or that my feature works.
I've updated the documentation with the relevant information (if needed).
I've added usage information (if the PR introduces new options)
I've included a "before" and "after" example to the description (if the PR is a user interface change).

DmitriyLewen · 2025-06-26T07:17:50Z

pkg/flag/aws_flags.go

shouldn't we delete the whole file?

nikpivkin · 2025-06-26T09:17:15Z

This PR removes the AWS region and endpoint flags used by the VM, so I think we will need to define for the VM its own group with these flags or leave the AWS group and remove the extra flags, but not use it in trivy-aws. I also found a bug: VM flags are initialised without the AWS endpoint flag:

trivy/pkg/commands/app.go

Lines 1109 to 1115 in 367564a

    
           &flag.AWSFlagGroup{ 
        
           	Region: &flag.Flag[string]{ 
        
           		Name:       "aws-region", 
        
           		ConfigName: "aws.region", 
        
           		Usage:      "AWS region to scan", 
        
           	}, 
        
           },

even though it is used when scanning the artifact:

trivy/pkg/fanal/artifact/vm/vm.go

Line 70 in 367564a

return newAMI(target, storage, opt.AWSRegion, opt.AWSEndpoint)

simar7 · 2025-06-27T00:57:08Z

This PR removes the AWS region and endpoint flags used by the VM, so I think we will need to define for the VM its own group with these flags or leave the AWS group and remove the extra flags, but not use it in trivy-aws. I also found a bug: VM flags are initialised without the AWS endpoint flag:

I saw that the VM command uses AWS Region so I kept it. But I'm not able to understand if it's defined here

trivy/pkg/commands/app.go

Lines 1100 to 1106 in 8c8475b

    
           &flag.AWSFlagGroup{ 
        
           	Region: &flag.Flag[string]{ 
        
           		Name:       "aws-region", 
        
           		ConfigName: "aws.region", 
        
           		Usage:      "AWS region to scan", 
        
           	}, 
        
           },

why do we need to still redefine the flag set?

It seems to show up in the options list. Are you saying the value will not get set?

trivy vm -h | grep region                          
      --aws-region string   AWS region to scan

simar7 · 2025-06-27T00:58:58Z

I also found a bug: VM flags are initialised without the AWS endpoint flag:

Safe to assume if we haven't seen a bug report all these years, it's not used much 😄

nikpivkin · 2025-06-27T05:14:18Z

It seems to show up in the options list. Are you saying the value will not get set?

The flag value will be set, but maybe we should leave the flag definition in the aws_flags file as we do for all other flags and initialize the AWS group via NewAWSFlagGroup? Also remove unnecessary flags from the AWSFlagGroup group and fields from the AWSOptions option.

DmitriyLewen · 2025-06-27T06:17:23Z

since we remove aws command and these 2 flags are used for vm mode - maybe create a vm_flags.go file and move the needed flags to that file?

in this case there will be no confusion and it will be immediately clear where these flags are used (they will also be easy to disable for other mods)

simar7 · 2025-06-27T06:23:32Z

since we remove aws command and these 2 flags are used for vm mode - maybe create a vm_flags.go file and move the needed flags to that file?

in this case there will be no confusion and it will be immediately clear where these flags are used (they will also be easy to disable for other mods)

It seems to show up in the options list. Are you saying the value will not get set?

The flag value will be set, but maybe we should leave the flag definition in the aws_flags file as we do for all other flags and initialize the AWS group via NewAWSFlagGroup? Also remove unnecessary flags from the AWSFlagGroup group and fields from the AWSOptions option.

Yes makes sense - I'll update the PR with that.

DmitriyLewen

You need to add new VM flags in

trivy/magefiles/docs.go

Line 194 in fd2bc91

}

and update config file

DmitriyLewen · 2025-06-30T06:08:33Z

pkg/flag/vm_flags.go

+package flag
+
+var (
+	awsRegionFlag = Flag[string]{


I think we can remove aws prefix now.

DmitriyLewen · 2025-06-30T06:09:36Z

pkg/flag/vm_flags.go

+var (
+	awsRegionFlag = Flag[string]{
+		Name:       "region",
+		ConfigName: "cloud.aws.region",


perhaps we want to change configName.
e.g.:

Suggested change

ConfigName: "cloud.aws.region",

ConfigName: "vm.region",

DmitriyLewen · 2025-06-30T06:12:48Z

pkg/flag/vm_flags.go

+}
+
+func (f *VMFlagGroup) Name() string {
+	return "AWS"


Suggested change

return "AWS"

return "VM"

knqyf263 · 2025-06-30T06:20:12Z

The AWS region flag is also used in image scanning.

trivy/pkg/fanal/image/registry/ecr/ecr.go

Line 33 in feaef96

    
           if option.AWSSecretKey != "" && option.AWSAccessKey != "" && option.AWSRegion != "" {

nikpivkin · 2025-06-30T06:28:46Z

Is it reasonable to remove aws from flags? The prefix provides more information about the purpose of the flag. Also, if support for another type of cloud artifact is added in the future that requires a region to be specified, flag naming may cause a conflict and flags may have to be renamed.

knqyf263 · 2025-06-30T06:44:17Z

I think we should keep AWS flags since image scanning already requires it, and, as @nikpivkin pointed out, trivy config might scan Helm charts in AWS ECR with some AWS configurations, for example.

simar7 · 2025-07-01T06:38:34Z

/magefiles/docs.go

Ah right - I missed this one. Updated a60d3e7

simar7 · 2025-07-08T06:47:02Z

@DmitriyLewen @knqyf263 @nikpivkin could you take another look? My understanding is that we're good with the AWS prefixes in the flag names for now, based on the above discussion.

DmitriyLewen · 2025-07-08T07:50:56Z

as correctly caught @knqyf263 and @nikpivkin we need to leave aws flags.
IIUC we need to make the following changes:

remove aws subcommand.
remove unused flags
add aws-region flag for image subcommand (this is our bug - users can configuration this flag)

simar7 · 2025-07-08T22:30:58Z

remove aws subcommand.

The aws subcommand has already been removed over a year ago, hence this PR. Are you referring to something else?

remove unused flags

Which would those be?

add aws-region flag for image subcommand (this is our bug - users can configuration this flag)

Can we create an issue for this and do it in a separate PR? The current PR is only refactoring (and removing) existing unused flags.

nikpivkin · 2025-07-09T03:34:52Z

Can we create an issue for this and do it in a separate PR? The current PR is only refactoring (and removing) existing unused flags.

I'll create an issue and a PR for it.

DmitriyLewen · 2025-07-09T06:38:13Z

The aws subcommand has already been removed over a year ago, hence this PR. Are you referring to something else?

Sorry for confusing.
I meant this one:

trivy/pkg/commands/app.go

Lines 115 to 124 in c6d4607

    
           // TODO(simar7): Only for backwards support guidance, delete the subcommand after a while. 
        
           if cmd, _, _ := rootCmd.Find([]string{"aws"}); cmd == cmd.Root() { // "trivy aws" not installed 
        
           	rootCmd.AddCommand(&cobra.Command{ 
        
           		Hidden: true, 
        
           		Long:   "Trivy AWS is now available as an optional plugin. See github.com/aquasecurity/trivy-aws for details.", 
        
           		Use:    "aws", 
        
           	}) 
        
           } 
        
           return rootCmd

Which would those be?

vm and image modes use aws-region flag.
vm uses aws-endpoint
aws-service, aws-skip-service, aws-account and aws-arn are not used.

knqyf263 · 2025-07-09T07:55:04Z

As we discussed above, the AWS flag may be used regardless of the target (image, VM, etc.), so it should belong to an independent flag group. I don't mind if it's an AWS flag group or a cloud flag group.

Additionally, removing the prefix would cause issues when specifying regions in other cloud environments, such as GCP or Azure. What do you think?

simar7 · 2025-07-09T20:42:05Z

The aws subcommand has already been removed over a year ago, hence this PR. Are you referring to something else?

Sorry for confusing. I meant this one:

trivy/pkg/commands/app.go

Lines 115 to 124 in c6d4607

// TODO(simar7): Only for backwards support guidance, delete the subcommand after a while.

if cmd, _, _ := rootCmd.Find([]string{"aws"}); cmd == cmd.Root() { // "trivy aws" not installed

rootCmd.AddCommand(&cobra.Command{

Hidden: true,

Long: "Trivy AWS is now available as an optional plugin. See github.com/aquasecurity/trivy-aws for details.",

Use: "aws",

})

}

return rootCmd

Isn't it already removed in this PR? https://github.com/aquasecurity/trivy/pull/9080/files#diff-f11fa87118181cac251936804d9f77a088aec34600e0774ea7303455723a9dff

As we discussed above, the AWS flag may be used regardless of the target (image, VM, etc.), so it should belong to an independent flag group. I don't mind if it's an AWS flag group or a cloud flag group.

Additionally, removing the prefix would cause issues when specifying regions in other cloud environments, such as GCP or Azure. What do you think?

Should we refactor once we have the need given other services using it? To me the scope of this PR was simply to remove unnecessary flags that aren't used today.

DmitriyLewen · 2025-07-10T05:34:59Z

Isn't it already removed in this PR? https://github.com/aquasecurity/trivy/pull/9080/files#diff-f11fa87118181cac251936804d9f77a088aec34600e0774ea7303455723a9dff

since some of the changes may have to be rolled back (for example, moving flags to a VM group), I wrote the necessary changes without taking into account the changes made.

knqyf263 · 2025-07-10T06:25:39Z

To me the scope of this PR was simply to remove unnecessary flags that aren't used today.

--aws-region is already used in image and VM scanning (although it may not properly work in image scanning). To minimize the scope of this PR, I think we should leave it as is.

Should we refactor once we have the need given other services using it?

Do you mean we will rename --aws-region to --region in this PR and change it back to --aws-region when we add support for other cloud services?

simar7 · 2025-07-11T05:25:26Z

@DmitriyLewen @knqyf263 - I've updated the scope for this PR to simply remove the helper text.

simar7 marked this pull request as ready for review June 26, 2025 02:13

simar7 requested a review from knqyf263 as a code owner June 26, 2025 02:13

simar7 mentioned this pull request Jun 26, 2025

fix(cli): add some values to the telemetry call #9056

Merged

4 tasks

knqyf263 approved these changes Jun 26, 2025

View reviewed changes

DmitriyLewen reviewed Jun 26, 2025

View reviewed changes

pkg/flag/aws_flags.go

Copy link

Contributor

DmitriyLewen Jun 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we delete the whole file?

simar7 force-pushed the remove-aws-flags branch 2 times, most recently from b04369b to b3050b8 Compare June 28, 2025 07:05

DmitriyLewen reviewed Jun 30, 2025

View reviewed changes

simar7 requested review from DmitriyLewen and nikpivkin July 1, 2025 06:39

refactor: remove aws flag helper message

044c533

simar7 force-pushed the remove-aws-flags branch from a60d3e7 to 044c533 Compare July 11, 2025 04:57

simar7 changed the title ~~refactor: remove aws flag options~~ refactor: remove aws flag helper message Jul 11, 2025

nikpivkin approved these changes Jul 11, 2025

View reviewed changes

knqyf263 approved these changes Jul 11, 2025

View reviewed changes

simar7 added this pull request to the merge queue Jul 11, 2025

Merged via the queue into aquasecurity:main with commit a822ace Jul 11, 2025
13 checks passed

simar7 deleted the remove-aws-flags branch July 11, 2025 06:41

yutatokoi pushed a commit to yutatokoi/trivy that referenced this pull request Aug 12, 2025

refactor: remove aws flag helper message (aquasecurity#9080)

e217bc3

refactor: remove aws flag helper message #9080

refactor: remove aws flag helper message #9080

Uh oh!

Conversation

simar7 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

DmitriyLewen Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

nikpivkin commented Jun 26, 2025

Uh oh!

simar7 commented Jun 27, 2025

Uh oh!

simar7 commented Jun 27, 2025

Uh oh!

nikpivkin commented Jun 27, 2025

Uh oh!

DmitriyLewen commented Jun 27, 2025

Uh oh!

simar7 commented Jun 27, 2025

Uh oh!

DmitriyLewen left a comment

Choose a reason for hiding this comment

Uh oh!

DmitriyLewen Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

DmitriyLewen Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

DmitriyLewen Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

knqyf263 commented Jun 30, 2025

Uh oh!

nikpivkin commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knqyf263 commented Jun 30, 2025

Uh oh!

simar7 commented Jul 1, 2025

Uh oh!

simar7 commented Jul 8, 2025

Uh oh!

DmitriyLewen commented Jul 8, 2025

Uh oh!

simar7 commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikpivkin commented Jul 9, 2025

Uh oh!

DmitriyLewen commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knqyf263 commented Jul 9, 2025

Uh oh!

simar7 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DmitriyLewen commented Jul 10, 2025

Uh oh!

knqyf263 commented Jul 10, 2025

Uh oh!

simar7 commented Jul 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

simar7 commented Jun 25, 2025 •

edited

Loading

nikpivkin commented Jun 30, 2025 •

edited

Loading

simar7 commented Jul 8, 2025 •

edited

Loading

DmitriyLewen commented Jul 9, 2025 •

edited

Loading

simar7 commented Jul 9, 2025 •

edited

Loading