Skip to content

Conversation

@spfink
Copy link
Contributor

@spfink spfink commented Jan 7, 2025

Problem

If a user was part of an AB group with a customization override, they could only ever use the customization from the override regardless of personal choice.

Solution

Changes the logic for when a customization will be overridden if the user is part of an AB group that has a customization override. With this change, if the user has manually selected a customization, it will not be overridden by the AB customization.


  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@spfink spfink requested review from a team as code owners January 7, 2025 18:12
@spfink spfink force-pushed the finks/default-custom branch 4 times, most recently from 4f1d757 to 318f09c Compare January 7, 2025 22:15
name: 'testCustomizationName',
description: 'testCustomizationDescription',
}

Copy link
Contributor

Choose a reason for hiding this comment

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

i feel it's better to have sinon.stub(FeatureConfigProvider, 'getFeature').returns(featureCustomization) here as well

Copy link
Contributor

Choose a reason for hiding this comment

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

to simulate the real case when there is an override and user pre-selected customization

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 to beforeEach so it's present for all tests

@Will-ShaoHua
Copy link
Contributor

thanks for adding test coverage

@spfink spfink force-pushed the finks/default-custom branch from 318f09c to 138fc93 Compare January 7, 2025 23:00
@Hweinstock
Copy link
Contributor

Is there a more concise way to describe this change in the PR title?

from lint-commits:

Invalid pull request title: `fix(amazonq): Change logic for customization override to not override if user has manually selected a customization`

* Problem: invalid subject (must be <=100 chars)
* Expected format: `type(scope): subject...`
    * type: one of (build, ci, config, deps, docs, feat, fix, perf, refactor, revert, style, telemetry, test, types)
    * scope: lowercase, <30 chars
    * subject: must be <[10](https://github.com/aws/aws-toolkit-vscode/actions/runs/12660729822/job/35287038708?pr=6316#step:5:11)0 chars
    * documentation: https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#pull-request-title
* Hint: *close and re-open the PR* to re-trigger CI (after fixing the PR title).

@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "Change logic for customization override to not override if user has manually selected a customization"
Copy link
Contributor

@hayemaxi hayemaxi Jan 8, 2025

Choose a reason for hiding this comment

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

Suggested change
"description": "Change logic for customization override to not override if user has manually selected a customization"
"description": "User-selected customizations are sometimes not being persisted."

or similar, see https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@spfink spfink changed the title fix(amazonq): Change logic for customization override to not override if user has manually selected a customization fix(amazonq): Prevent customization override if user has manually selected a customization Jan 8, 2025
@spfink spfink force-pushed the finks/default-custom branch from 138fc93 to a1a4c1c Compare January 8, 2025 16:40
@spfink spfink closed this Jan 8, 2025
@spfink spfink reopened this Jan 8, 2025
@spfink spfink force-pushed the finks/default-custom branch from a1a4c1c to 409d575 Compare January 8, 2025 16:45
@spfink spfink requested a review from hayemaxi January 8, 2025 17:04
@spfink spfink force-pushed the finks/default-custom branch from 409d575 to a211ce6 Compare January 8, 2025 18:25
@spfink spfink requested a review from justinmk3 January 8, 2025 18:30
@spfink spfink force-pushed the finks/default-custom branch from a211ce6 to 78ece02 Compare January 8, 2025 18:51
@spfink spfink force-pushed the finks/default-custom branch from 78ece02 to ec9803f Compare January 8, 2025 19:43
@spfink
Copy link
Contributor Author

spfink commented Jan 8, 2025

Lots of connection resets in the test runs today, will try another push to see if we can get them running again but they are passing locally and passed previously.

@hayemaxi
Copy link
Contributor

/retryBuilds

@hayemaxi hayemaxi merged commit c267457 into aws:master Jan 13, 2025
17 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.

5 participants