-
Notifications
You must be signed in to change notification settings - Fork 37
Updated Dockerfile to resolve build issues due to vintage #131
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
Conversation
Merge refactor --------- Co-authored-by: Richard Worwood <richard.worwood@endava.com>
* Update supported_versions.json to latest * Update supported_versions.json * Update supported_versions.json * Update supported_versions.json * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfile * Update Dockerfil formatting and safety * Push update to Dockerfile * specify devian version * add gosu * remove gosu * switch to curl * specify azure-cli version * simplify structure * update versions * change setuptools version * switch to 58 * uses DOCKERHUB_USERNAME secret as * correct push name --------- Co-authored-by: Richard Worwood <richard.worwood@endava.com>
* Update to push using secrets * Update Dockerfile "Remove line" --------- Co-authored-by: Richard Worwood <richard.worwood@endava.com>
Bumps [plexsystems/container-structure-test-action](https://github.com/plexsystems/container-structure-test-action) from 0.2.0 to 0.3.0. - [Release notes](https://github.com/plexsystems/container-structure-test-action/releases) - [Commits](plexsystems/container-structure-test-action@v0.2.0...v0.3.0) --- updated-dependencies: - dependency-name: plexsystems/container-structure-test-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 2 to 3. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v2...v3) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/download-artifact](https://github.com/actions/download-artifact) from 2 to 3. - [Release notes](https://github.com/actions/download-artifact/releases) - [Commits](actions/download-artifact@v2...v3) --- updated-dependencies: - dependency-name: actions/download-artifact dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
...#9) * Create supported-versions.yml * Create test-harness for testing supported versions workflow * Update build-test.yml * Update supported-versions.yml * Update test-harness.yml * Update test-harness.yml * commit test for supported-versions workflow * test for matrix content * add latest version workflow * move from includes/ directory and rename to includes_ * update to use include_latest-version.yml --------- Co-authored-by: Richard Worwood <richard.worwood@endava.com>
...s using artifact repository (#11) * Simplified build-test.yml to remove need to upload and download images to artifact repository
bgauduch
commented
Aug 23, 2023
Hello @Jubblin 👋
Thanks a lot for the incredible work injected into this PR 🤩
I'm very sorry for the answer delay and will start the review right now.
As a matter of fact, I'm looking for maintainers for this project (and also for the AWS sibling) !
Would you be interested to work more closely on those subjects ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What a nice work @Jubblin , thanks a lot 🙏
Many questions and comments after this review 🙂
I will keep an eye closely on this MR to improve the project ASAP 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this Dockerfile follow the multi-stages build pattern :
- Optimizing throw-away stages is not required, as additional image layers won't end up in the final runtime stage
- Keeping each instruction in a separated layer allow a more efficient use of build caching
So I would suggest to revert this changes 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above here too 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you changes this to test the badges and images, but we want to keep upstream references on https://github.com/zenika-open-source/terraform-azure-cli here 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, we want to keep the reference on Zenika registry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same feedback as in push-latest workflow, we might be better off keeping ORGANIZATION env var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I didn't understand correctly, but aren't include_latest-version and include_supported-versions.yml a bit of a duplication ?
The only difference I can sport is sort | reverse VS sort | .[-1] for both TF and AWSCLI versions, I'm not sure to quite understand the use case.
EDIT :
Oh okay the difference is the latest version for each VS a matrix of all versions for each tool.
In that cas, a plural on outputs would be nice 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware of references elsewhere in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beware of references elsewhere in the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you stated in the name, it might be better to keep the file name plural as latest version is exposed for multi tools, what do you think ?
That would be: .github/workflows/include_latest-versions.yml
Updated Dockerfile to resolve Dockerfile build issues, updated actions to the latest versions, and made workflows more generic so they can be built on more simply by others moving forward.