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.

Sign up for GitHub

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

Jump to bottom

AAE-10135: turn build-and-tag-maven GH reusable workflow into an acti… #105

Merged
merged 2 commits into from
Oct 5, 2022
Merged

AAE-10135: turn build-and-tag-maven GH reusable workflow into an acti… #105

merged 2 commits into from
Oct 5, 2022

Conversation

atchertchian
Copy link
Contributor

@atchertchian atchertchian commented Jul 29, 2022 •
edited
Loading

…on

@pr-triage pr-triage bot added the PR: draft label Jul 29, 2022
@erdemedeiros erdemedeiros marked this pull request as ready for review September 15, 2022 15:40
@erdemedeiros erdemedeiros marked this pull request as draft September 15, 2022 15:41
@atchertchian
Copy link
Contributor Author

Will rework the PR as some of the changes were already handled with #114

@atchertchian atchertchian force-pushed the AAE-10135-change-maven-build-workflow-to-action branch from 1897fad to 04b5d2c Compare September 21, 2022 15:00
@atchertchian atchertchian force-pushed the AAE-10135-change-maven-build-workflow-to-action branch 2 times, most recently from c6fd37b to d5cae64 Compare September 21, 2022 17:12
@atchertchian atchertchian marked this pull request as ready for review September 21, 2022 17:28
@atchertchian
Copy link
Contributor Author

Will merge one PR using this action (to check push logic) before merging this PR.

gionn
gionn previously approved these changes Sep 26, 2022
View reviewed changes
@atchertchian atchertchian force-pushed the AAE-10135-change-maven-build-workflow-to-action branch from d5cae64 to 97c3aac Compare September 27, 2022 10:43
@atchertchian
Copy link
Contributor Author

force pushed to handle conflict after merge of #126

erdemedeiros
erdemedeiros previously approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@erdemedeiros erdemedeiros left a comment

Choose a reason for hiding this comment

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

LGTM!

@atchertchian atchertchian force-pushed the AAE-10135-change-maven-build-workflow-to-action branch from 97c3aac to 5a809d7 Compare October 3, 2022 09:07
@atchertchian
Copy link
Contributor Author

force pushed to handle conflict after latest release

@gionn gionn dismissed stale reviews from erdemedeiros and themself via 5a809d7 October 4, 2022 07:20
gionn
gionn reviewed Oct 4, 2022
View reviewed changes
.github/actions/maven-build-and-tag/action.yml Outdated Show resolved Hide resolved
.github/actions/maven-build-and-tag/action.yml
Comment on lines +24 to +35
quay-username:
description: Quay.io user name
required: true
quay-password:
description: Quay.io password
required: true
docker-username:
description: Docker.io user name
required: true
docker-password:
description: Docker.io password
required: true
Copy link
Member

Choose a reason for hiding this comment

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

maybe those could be optional since it depends on the packages you are building if they need to push to quay or docker or both

you could also skip docker-login steps depending on those inputs empty or not (otherwise they will fail)

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, we always need to login to Docker Hub, even when we are pushing images to quay.io, that's because some base images and images used by integration tests are hosted in Docker Hub and without login, we run out of quota to pull those images.

Copy link
Member

Choose a reason for hiding this comment

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

yeah for your specific use case it's fine but I am always thinking if there is more room to make this action reusable in different contexts

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense to make it as reusable as possible, but I prefer to minimize the amount of work without anticipating too much. The risk is spending time adding logic that will never be used. We can always evolve the implementation to cover new scenarios when they are needed. Does it make sense?

erdemedeiros
erdemedeiros previously approved these changes Oct 4, 2022
View reviewed changes
Copy link
Contributor

@erdemedeiros erdemedeiros left a comment

Choose a reason for hiding this comment

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

LGMT!

@atchertchian atchertchian merged commit 1c7b179 into master Oct 5, 2022
@atchertchian atchertchian deleted the AAE-10135-change-maven-build-workflow-to-action branch October 5, 2022 09:02
@atchertchian atchertchian restored the AAE-10135-change-maven-build-workflow-to-action branch October 5, 2022 09:02
@atchertchian
Copy link
Contributor Author

Keeping the work branch for now as there are currently running workflows that are still running on it

@gionn gionn deleted the AAE-10135-change-maven-build-workflow-to-action branch October 18, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gionn gionn gionn approved these changes

@erdemedeiros erdemedeiros Awaiting requested review from erdemedeiros

Assignees
No one assigned
Labels
PR: merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atchertchian @gionn @erdemedeiros