Skip to content

feat(server): Implement and document security group attach/detach commands#1341

Draft
s-inter wants to merge 8 commits intomainfrom
feat/server-security-group-attach-and-detach
Draft

feat(server): Implement and document security group attach/detach commands#1341
s-inter wants to merge 8 commits intomainfrom
feat/server-security-group-attach-and-detach

Conversation

@s-inter
Copy link

@s-inter s-inter commented Mar 13, 2026

Description

This PR implements the stackit server security-group attach and detach commands.

Changes

  • Added stackit server security-group attach to associate a security group with a server.
  • Added stackit server security-group detach to remove a security group from a server.
  • Added unit tests for input parsing and request building.
  • Generated updated command documentation.

Testing Instructions

  1. Preconditions: have an existing server and a security group in the same project/region.
  2. Build:
make build
  1. Attach:
./bin/stackit server security-group attach --server-id <SERVER_ID> --security-group-id <SG_ID>
  1. Verify attach via server describe:
./bin/stackit server describe <SERVER_ID> --output-format json | jq '[.nics[].securityGroups[]]'

Confirm the security group appears in the server details.

  1. Detach:
./bin/stackit server security-group detach --server-id <SERVER_ID> --security-group-id <SG_ID>
  1. Verify detach via server describe and confirm the security group is no longer associated.

Related Issues

Resolves #1216

relates to STACKITCLI-308

Checklist

  • Issue was linked above
  • Code format was applied: make fmt
  • Examples were added / adjusted (see e.g. here)
  • Docs are up-to-date: make generate-docs (will be checked by CI)
  • Unit tests got implemented or updated
  • Unit tests are passing: make test (will be checked by CI)
  • No linter issues: make lint (will be checked by CI)

@github-actions
Copy link

Merging this branch will increase overall coverage

Impacted Packages Coverage Δ 🤖
github.com/stackitcloud/stackit-cli/internal/cmd/server 0.00% (ø)
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group 0.00% (ø)
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/attach 36.59% (+36.59%) 🌟
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/detach 36.59% (+36.59%) 🌟
github.com/stackitcloud/stackit-cli/internal/cmd/server/service-account/detach 45.00% (ø)

Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/attach/attach.go 36.59% (+36.59%) 41 (+41) 15 (+15) 26 (+26) 🌟
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/detach/detach.go 36.59% (+36.59%) 41 (+41) 15 (+15) 26 (+26) 🌟
github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/security-group.go 0.00% (ø) 5 (+5) 0 5 (+5)
github.com/stackitcloud/stackit-cli/internal/cmd/server/server.go 0.00% (ø) 26 (+1) 0 26 (+1)
github.com/stackitcloud/stackit-cli/internal/cmd/server/service-account/detach/detach.go 45.00% (ø) 40 18 22

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/attach/attach_test.go
  • github.com/stackitcloud/stackit-cli/internal/cmd/server/security-group/detach/detach_test.go

}

prompt := fmt.Sprintf("Are your sure you want to detach service account %q from a server %q?", model.ServiceAccMail, serverLabel)
prompt := fmt.Sprintf("Are you sure you want to detach service account %q from a server %q?", model.ServiceAccMail, serverLabel)
Copy link
Author

Choose a reason for hiding this comment

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

Typo fix (boyscout) - okay to keep in here?

Copy link
Member

Choose a reason for hiding this comment

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

totally fine, thanks

@s-inter s-inter linked an issue Mar 13, 2026 that may be closed by this pull request
return fmt.Errorf("attach security group to server: %w", err)
}

params.Printer.Info("Attached security group %q to server %q\n", securityGroupLabel, serverLabel)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
params.Printer.Info("Attached security group %q to server %q\n", securityGroupLabel, serverLabel)
params.Printer.Outputf("Attached security group %q to server %q\n", securityGroupLabel, serverLabel)

This is often handled wrong in the codebase, that's why I'll explain it in detail. Please make sure to remember this in the future and especially look out for it in code reviews you will do in the future. 😅

Outputf prints to stdout, Info prints to stderr. See the code examples below for details.

Since this output here is a success message and no error message, it belongs into the stdout output from my perspective.

// Print an output using Printf to the defined output (falling back to Stderr if not set).
// If output format is set to none, it does nothing
func (p *Printer) Outputf(msg string, args ...any) {
outputFormat := viper.GetString(outputFormatKey)
if outputFormat == NoneOutputFormat {
return
}
p.Cmd.Printf(msg, args...)
}

// Print an Info level output to the defined Err output (falling back to Stderr if not set).
// If the verbosity level is not Debug or Info, it does nothing.
func (p *Printer) Info(msg string, args ...any) {
if !p.IsVerbosityDebug() && !p.IsVerbosityInfo() {
return
}
p.Cmd.PrintErrf(msg, args...)
}


for _, tt := range tests {
t.Run(tt.description, func(t *testing.T) {
p := print.NewPrinter()
Copy link
Member

Choose a reason for hiding this comment

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

testutils.TestParseInput(t, NewCmd, parseInput, tt.expectedModel, tt.argValues, tt.flagValues, tt.isValid)

// TestParseInput centralizes the logic to test a combination of inputs (arguments, flags) for a cobra command
func TestParseInput[T any](t *testing.T, cmdFactory func(*types.CmdParams) *cobra.Command, parseInputFunc func(*print.Printer, *cobra.Command, []string) (T, error), expectedModel T, argValues []string, flagValues map[string]string, isValid bool) {
t.Helper()
TestParseInputWithAdditionalFlags(t, cmdFactory, parseInputFunc, expectedModel, argValues, flagValues, map[string][]string{}, isValid)
}
// TestParseInputWithAdditionalFlags centralizes the logic to test a combination of inputs (arguments, flags) for a cobra command.
// It allows to pass multiple instances of a single flag to the cobra command using the `additionalFlagValues` parameter.
func TestParseInputWithAdditionalFlags[T any](t *testing.T, cmdFactory func(*types.CmdParams) *cobra.Command, parseInputFunc func(*print.Printer, *cobra.Command, []string) (T, error), expectedModel T, argValues []string, flagValues map[string]string, additionalFlagValues map[string][]string, isValid bool) {
TestParseInputWithOptions(t, cmdFactory, parseInputFunc, expectedModel, argValues, flagValues, additionalFlagValues, isValid, nil)
}
func TestParseInputWithOptions[T any](t *testing.T, cmdFactory func(*types.CmdParams) *cobra.Command, parseInputFunc func(*print.Printer, *cobra.Command, []string) (T, error), expectedModel T, argValues []string, flagValues map[string]string, additionalFlagValues map[string][]string, isValid bool, testingOptions []TestingOption) {

Please use this util funcs to save us the boilerplate code

return fmt.Errorf("detach security group from server: %w", err)
}

params.Printer.Info("Detached security group %q from server %q\n", securityGroupLabel, serverLabel)
Copy link
Member

Choose a reason for hiding this comment

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

same here


for _, tt := range tests {
t.Run(tt.description, func(t *testing.T) {
p := print.NewPrinter()
Copy link
Member

Choose a reason for hiding this comment

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

same here

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.

IaaS: add security group to server not supported

2 participants