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: Dockerfile typo #124

Merged
merged 5 commits into from
May 6, 2024
Merged

fix: Dockerfile typo #124

merged 5 commits into from
May 6, 2024

Conversation

justas05
Copy link
Contributor

Issue #, if available:

Description of changes: not required

Testing done:

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

  • I have read the CONTRIBUTING doc
  • I used the commit message format described in CONTRIBUTING
  • I have updated any necessary documentation, including READMEs and comments (where appropriate)

Tests

  • I have added tests that prove my fix is effective or that my feature works (if appropriate)
  • I have checked that my tests are not configured for a specific environment

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

@jamolina
Copy link
Contributor

Thank you for your contribution @justas05 ! We will review it and let you know if we have any questions.

@justas05 justas05 requested a review from jamolina April 17, 2024 06:39
@justas05
Copy link
Contributor Author

@jamolina, can you help me build in docker?
Ubuntu-22.04

17.14 /root/credentials-fetcher/api/tests/gmsa_test_client.cpp:750:13: error: use of undeclared identifier 'retrieve_credspec_from_s3_test'
17.14             retrieve_credspec_from_s3_test();
17.14             ^
17.16 /root/credentials-fetcher/api/tests/gmsa_test_client.cpp:751:13: error: use of undeclared identifier 'retrieve_credspec_from_secrets_manager_test'
17.16             retrieve_credspec_from_secrets_manager_test();
17.16             ^
17.63 1 warning and 2 errors generated.
17.65 make[2]: *** [api/tests/CMakeFiles/gmsa_test_client.dir/build.make:79: api/tests/CMakeFiles/gmsa_test_client.dir/gmsa_test_client.cpp.o] Error 1
17.65 make[2]: Leaving directory '/root/credentials-fetcher/build'
17.65 make[1]: *** [CMakeFiles/Makefile2:274: api/tests/CMakeFiles/gmsa_test_client.dir/all] Error 2
17.65 make[1]: *** Waiting for unfinished jobs....
25.62 make[2]: Leaving directory '/root/credentials-fetcher/build'
25.62 [ 94%] Built target credentials-fetcherd
25.63 make[1]: Leaving directory '/root/credentials-fetcher/build'
25.63 make: *** [Makefile:149: all] Error 2

@jamolina
Copy link
Contributor

Hey @justas05, please pull the latest version to get the fix that got merged today. Also, I'm curious about the last commit on this PR. Was there a reason to change the remove the cd command and the relative paths on the file?

@justas05
Copy link
Contributor Author

justas05 commented Apr 26, 2024

Hey @jamolina, there is an error when running this command.

cd /root
git clone ... grpc
cd build
...
cd grpc/

In the case of an absolute path, we are confident that we'll end up in the correct directory.

Copy link
Contributor

@jamolina jamolina left a comment

Choose a reason for hiding this comment

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

Ubuntu 22.04 builds successfully, but I see an issue with Ubuntu 20.04. Could you check my comment?

docker/Dockerfile-ubuntu-20.04 Outdated Show resolved Hide resolved
Copy link
Contributor

@jamolina jamolina 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 your patience during this process. Changes look good. Thanks again for your contribution!

@jamolina jamolina merged commit 993cc8c into aws:mainline May 6, 2024
1 check 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.

None yet

4 participants