Skip to content

feat: add toggle flag step#111

Merged
k3llymariee merged 6 commits into
mainfrom
kelly/sc-238066/add-toggle-flag
Apr 3, 2024
Merged

feat: add toggle flag step#111
k3llymariee merged 6 commits into
mainfrom
kelly/sc-238066/add-toggle-flag

Conversation

@k3llymariee
Copy link
Copy Markdown
Contributor

@k3llymariee k3llymariee commented Apr 3, 2024

Adds the remaining functionality for the toggle flag steps. Error handling will follow in a separate PR.

@shortcut-integration
Copy link
Copy Markdown

This pull request has been linked to Shortcut Story #238066: add toggle flag page to setup wizard.

@k3llymariee k3llymariee changed the title Kelly/sc 238066/add toggle flag feat: (real) add toggle flag step Apr 3, 2024
Comment thread cmd/flags/update.go
Comment on lines +126 to +127
envKey := viper.GetString(cliflags.EnvironmentFlag)
patch = flags.BuildToggleFlagPatch(envKey, cmd.CalledAs() == "toggle-on")
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

refactored this to play better with where we call this in the quickstart

Comment thread cmd/flags/update_test.go
})
}

func TestToggle(t *testing.T) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added tests to make sure i didn't break anything

accessToken string
baseUri string
currentModel tea.Model
sdkKind string
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

like flagKey storing anything we might need in a subsequent step on the container model

Comment on lines -56 to -57
accessToken := viper.GetString(cliflags.AccessTokenFlag)
baseUri := viper.GetString(cliflags.BaseURIFlag)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

removed this since we have this at the container level/should set these values on the createFlagModel

@k3llymariee k3llymariee marked this pull request as ready for review April 3, 2024 16:31
@k3llymariee k3llymariee changed the title feat: (real) add toggle flag step feat: add toggle flag step Apr 3, 2024

type toggledFlagMsg struct{}

func sendToggleFlagMsg(client flags.Client, accessToken, baseUri, flagKey string, enabled bool) tea.Cmd {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does it make more sense to name this after the message it sends, so in this case it would be sendToggledFlagMsg (past tense)? I could see it implicitly sending a toggleFlagMsg and returning the results, either a toggledFlagMsg or errMsg, but I could also see that as confusing because there isn't a specific toggleFlagMsg type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ya I did actually get a bit tripped on this looking for the the actual msg type it specified when I picked it back up again, but it is also Doing The Thing here

Comment thread internal/quickstart/toggle_flag.go Outdated
accessToken string
baseUri string
client flags.Client
enabled bool // whether the flag has ever been toggled on
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you rename this something like flagWasToggled since I think that enabled means the flag is currently toggled on and not that it was changed at all.

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.

2 participants