Skip to content

Conversation

@neilk-aws
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Description

Improves the formatting and strings presented to customers when the allow /dev to run code and test commands automatically. Also includes minor variable name changes to improve internal consistency.

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes
  • A short description of the change has been added to the CHANGELOG if the change is customer-facing in the IDE.
  • I have added metrics for my changes (if required)

License

I confirm that my contribution is made under the terms of the Apache 2.0 license.

@neilk-aws neilk-aws requested review from a team as code owners January 28, 2025 06:31
@neilk-aws neilk-aws changed the title chore(amazonq): Update autobuild setting strings docs(amazonq): Update autobuild setting strings Jan 28, 2025
val stringValue by map<CodeWhispererStringConfigurationType, String>()

val projectAutoBuildConfigurationMap by map<String, Boolean>()
val autoBuildSetting by map<String, Boolean>()
Copy link
Contributor

@rli rli Jan 28, 2025

Choose a reason for hiding this comment

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

needs @get:Property like the other properties in here
after release, renaming this will be a breaking change for customers so it is probably worth a test case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the missing @get:Property. Do you have a recommendation on how to test that we aren't renaming a variable? I recognize renaming this would be breaking and is only done now since we are pre-release.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh great there are no existing tests on this state component
basic ser/deser tests similar to below should be sufficient coverage:

@Test
fun serialize() {
val element = xmlElement(
"""
<component name="codewhispererCustomizationStates">
</component>
""".trimIndent()
)
val state = CodeWhispererCustomizationState().apply {
this.previousAvailableCustomizations.putAll(
mapOf(
"fake-sso-url" to mutableListOf("arn_1", "arn_2")
)
)
this.connectionIdToActiveCustomizationArn.putAll(
mapOf(
"fake-sso-url" to CodeWhispererCustomization(arn = "arn_2", name = "name_2", description = "description_2")
)
)
}
XmlSerializer.serializeInto(state, element)
val actual = XMLOutputter().outputString(element)
val expected = "<component name=\"codewhispererCustomizationStates\">\n" +
"<option name=\"connectionIdToActiveCustomizationArn\">" +
"<map>" +
"<entry key=\"fake-sso-url\">" +
"<value>" +
"<CodeWhispererCustomization>" +
"<option name=\"arn\" value=\"arn_2\" />" +
"<option name=\"name\" value=\"name_2\" />" +
"<option name=\"description\" value=\"description_2\" />" +
"</CodeWhispererCustomization>" +
"</value>" +
"</entry>" +
"</map>" +
"</option>" +
"<option name=\"previousAvailableCustomizations\">" +
"<map>" +
"<entry key=\"fake-sso-url\">" +
"<value>" +
"<list>" +
"<option value=\"arn_1\" />" +
"<option value=\"arn_2\" />" +
"</list>" +
"</value>" +
"</entry>" +
"</map>" +
"</option>" +
"</component>"
assertThat(actual).isEqualTo(expected)
}
@Test
fun `deserialize empty data`() {
val element = xmlElement(
"""
<component name="codewhispererCustomizationStates">
</component>
"""
)
val actual = XmlSerializer.deserialize(element, CodeWhispererCustomizationState::class.java)
assertThat(actual.connectionIdToActiveCustomizationArn).hasSize(0)
assertThat(actual.previousAvailableCustomizations).hasSize(0)
}
@Test
fun `deserialize users choosing a customization`() {
val element = xmlElement(
"""
<component name="codewhispererCustomizationStates">
<option name="connectionIdToActiveCustomizationArn">
<map>
<entry key="fake-sso-url">
<value>
<CodeWhispererCustomization>
<option name="arn" value="arn_2" />
<option name="name" value="name_2" />
<option name="description" value="description_2" />
</CodeWhispererCustomization>
</value>
</entry>
</map>
</option>
<option name="previousAvailableCustomizations">
<map>
<entry key="fake-sso-url">
<value>
<list>
<option value="arn_1" />
<option value="arn_2" />
<option value="arn_3" />
</list>
</value>
</entry>
</map>
</option>
</component>
"""
)
val actual = XmlSerializer.deserialize(element, CodeWhispererCustomizationState::class.java)
assertThat(actual.connectionIdToActiveCustomizationArn).hasSize(1)
assertThat(actual.connectionIdToActiveCustomizationArn["fake-sso-url"]).isEqualTo(
CodeWhispererCustomization(
arn = "arn_2",
name = "name_2",
description = "description_2"
)
)
assertThat(actual.previousAvailableCustomizations).hasSize(1)
assertThat(actual.previousAvailableCustomizations["fake-sso-url"]).isEqualTo(listOf("arn_1", "arn_2", "arn_3"))
}
@Test
fun `deserialize users choosing default customization`() {
val element = xmlElement(
"""
<component name="codewhispererCustomizationStates">
<option name="previousAvailableCustomizations">
<map>
<entry key="fake-sso-url">
<value>
<list>
<option value="arn_1" />
<option value="arn_2" />
<option value="arn_3" />
</list>
</value>
</entry>
</map>
</option>
</component>
"""
)
val actual = XmlSerializer.deserialize(element, CodeWhispererCustomizationState::class.java)
assertThat(actual.connectionIdToActiveCustomizationArn).hasSize(0)
assertThat(actual.previousAvailableCustomizations).hasSize(1)
assertThat(actual.previousAvailableCustomizations["fake-sso-url"]).isEqualTo(listOf("arn_1", "arn_2", "arn_3"))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

on another note, doing a map of project -> boolean isnt really the best way to model this.

if you don't want the setting to be application-wide, you should define a new project-level state component so that the setting is saved per-project. right now the implementation looks like it makes a new check box for every project the user opens?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The setting is application wide and mirrors the structure that we're able to give in VSCode. I think there is opportunity for improvement in modeling, but I don't think I want to block on that right now. I'd love to learn about where project-level state component and what other features are using it. If we do migrate, we'll implement the logic to import the settings accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointer on the tests. I've added them here, with an initial focus on our property. I've kept it generic for other teams to be able to contribute more validation there.

Copy link
Contributor

Choose a reason for hiding this comment

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

switching between the two is not exactly trivial so it would be better to get that right now.

the configurable is already set up to support project-level settings so the only difference would be creating a new state service, and you can get rid of the weird map layer

    private val codeWhispererProjectSettings
        get() = CodeWhispererProjectSettings.getInstance(project)
            group(message("aws.settings.codewhisperer.feature_development")) {
                row {
                    text(message("aws.settings.codewhisperer.feature_development.allow_running_code_and_test_commands"))
                }
                row {
                    checkBox(codeWhispererProjectSettings.getAutoBuildSetting()).apply {
                        connect.subscribe(
                            ToolkitConnectionManagerListener.TOPIC,
                            object : ToolkitConnectionManagerListener {
                                override fun activeConnectionChanged(newConnection: ToolkitConnection?) {
                                    enabled(isCodeWhispererEnabled(project))
                                }
                            }
                        )

                        bindSelected(
                            getter = { codeWhispererProjectSettings.isAutoBuildFeatureEnabled() },
                            setter = { newValue -> codeWhispererProjectSettings.toggleAutoBuildFeature(newValue) }
                        )
                    }
                }
            }    
@Service(Level.PROJECT)
@State(name = "codewhispererProejctSettings", storages = [Storage("aws.xml")])
class CodeWhispererProjectSettings(private val project: Project) : PersistentStateComponent<CodeWhispererProjectConfiguration> {
...
}

class CodeWhispererProjectConfiguration : BaseState() {
    @get:Property
    val autoBuildSetting by property(false)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can take this as a follow up. Given we're trying to merge the feature branch into main today, I think we'll accept the risk of needing to implement a pathway to import settings from the storage layer from the existing format into a the new format once we migrate to it.

Copy link
Contributor

Choose a reason for hiding this comment

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

sure

@rli rli merged commit 86e993e into aws:feature/dev-execution Jan 28, 2025
2 of 9 checks passed
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