Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix an issue where sections were not properly divided when parsing the config file #1216

Merged
merged 6 commits into from
Feb 21, 2024

Conversation

jeonghwan-jang
Copy link
Contributor

@jeonghwan-jang jeonghwan-jang commented Feb 15, 2024

Issue #

Description of changes

When parsing sections in a config file, duplication between items with and without a section prefix is being removed. At this time, if different sections have the same name, a problem occurs where they are removed.

For example, the default profile is removed from the config file below.

[default]
sso_session = default
sso_account_id = 111122223333
sso_role_name = SampleRole
region = us-east-1
output = json

[sso-session default]
sso_region = us-east-1
sso_start_url = https://provided-domain.awsapps.com/start
sso_registration_scopes = sso:account:access

It seems that the logic for verifying section duplication is based on before the config file supports various section types.
Therefore, we modified it to check the type of the section as well when removing duplicates.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jeonghwan-jang jeonghwan-jang requested a review from a team as a code owner February 15, 2024 06:38
@lauzadis
Copy link
Member

Hi, thanks for the contribution! Because we are still refining our external contribution process, those e2e-tests and service-check-batch tasks are expected to fail. We'll run them manually ourselves and confirm they pass before merging the PR.

It would be nice to have a test that confirms this behavior is fixed, would you be able to add an entry to our parser test suite? If you have trouble or aren't interested in doing that, let me know and I can work on adding it.

@jeonghwan-jang
Copy link
Contributor Author

Hi, thanks for the contribution! Because we are still refining our external contribution process, those e2e-tests and service-check-batch tasks are expected to fail. We'll run them manually ourselves and confirm they pass before merging the PR.

It would be nice to have a test that confirms this behavior is fixed, would you be able to add an entry to our parser test suite? If you have trouble or aren't interested in doing that, let me know and I can work on adding it.

Hi, thank you for reviewing PR.
I have added the parser test suite you mentioned.
After adding, we confirmed that it did not pass in the existing logic and that it passed in the modified logic.

Copy link
Member

@lauzadis lauzadis left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test. I confirmed it works and ran the extra CI in #1219, it all passed. Thanks for the contribution!

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jeonghwan-jang
Copy link
Contributor Author

jeonghwan-jang commented Feb 21, 2024 via email

@aws-sdk-kotlin-ci aws-sdk-kotlin-ci merged commit 4153e02 into awslabs:main Feb 21, 2024
12 of 15 checks passed
@lauzadis
Copy link
Member

Yes, sorry I overlooked this. It should be available in tomorrow's release, v1.0.63.

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.

3 participants