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

Add Account Id to Credentials #260

Merged
merged 8 commits into from
Jan 27, 2025
Merged

Add Account Id to Credentials #260

merged 8 commits into from
Jan 27, 2025

Conversation

waahm7
Copy link
Contributor

@waahm7 waahm7 commented Jan 13, 2025

Issue #, if available:
awslabs/aws-crt-swift#303

Description of changes:

  • Adds a new optional account_id field to credentials. Currently, only static credentials support it, but soon we will add account_id for SSO, STS, and any other providers that can provide it.
  • Code coverage action was broken, fixed by updating gcc to match newer ubuntu. Because we updated the action to use newer ubuntu 24 image, we were getting an error that
Problem running coverage on file: /home/runner/work/aws-c-s3/aws-c-s3/build/aws-c-s3/CMakeFiles/aws-c-s3.dir/source/s3_checksum_stream.c.gcda
Command produced error: /home/runner/work/aws-c-s3/aws-c-s3/build/aws-c-s3/CMakeFiles/aws-c-s3.dir/source/s3_checksum_stream.c.gcno:version 'A95*', prefer 'B33*

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

@waahm7 waahm7 changed the title Add account id Add Account Id to Credentials Jan 13, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 80.50%. Comparing base (cdfb616) to head (ff1ba7a).
Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
source/credentials.c 93.75% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
+ Coverage   80.41%   80.50%   +0.09%     
==========================================
  Files          33       33              
  Lines        6019     6099      +80     
==========================================
+ Hits         4840     4910      +70     
- Misses       1179     1189      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -28,4 +28,4 @@ jobs:
run: |
python3 -c "from urllib.request import urlretrieve; urlretrieve('${{ env.BUILDER_HOST }}/${{ env.BUILDER_SOURCE }}/${{ env.BUILDER_VERSION }}/builder.pyz?run=${{ env.RUN }}', 'builder')"
chmod a+x builder
./builder build -p ${{ env.PACKAGE_NAME }} --compiler=gcc-9 --cmake-extra=-DASSERT_LOCK_HELD=ON --coverage
./builder build -p ${{ env.PACKAGE_NAME }} --compiler=gcc-12 --cmake-extra=-DASSERT_LOCK_HELD=ON --coverage
Copy link
Contributor

Choose a reason for hiding this comment

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

why? Add some comments about this?
Do we need it to be applied to other codecov jobs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the comment. Yes, we need to fix it everywhere where we updated the code coverage action ubuntu version.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm looks like the ci before the commit changing the compiler still work? https://github.com/awslabs/aws-c-auth/actions/runs/12757805423/job/35558824650

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you expand the build step and scroll to the end, you can see the errors. No idea why the overall CI says success.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, maybe instead of updating the gcc version for every codecov ci, we should update our builder

https://github.com/awslabs/aws-crt-builder/blob/42d15ba4ee3fbe0e812e25094a8cf59eaef0cf1e/builder/actions/cmake.py#L254-L258

We install our own version of gcc, but the ctest -T coverage probably still use the default one in the system.

Seems like we can pass --gcov to chose what version of gcov to solve the version mismatch thing. https://stackoverflow.com/questions/12454175/gcov-out-of-memory-mismatched-version

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, fixed by awslabs/aws-crt-builder#317.

@waahm7 waahm7 requested a review from TingDaoK January 24, 2025 21:40
@waahm7 waahm7 merged commit 274a1d2 into main Jan 27, 2025
34 checks passed
@waahm7 waahm7 deleted the account-id-in-creds branch January 27, 2025 18:56
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