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

AFI-1520:Externalize required parameters for build in build-and-relea… #255

Merged
merged 8 commits into from
May 4, 2023

Conversation

SushmitaGoswami
Copy link
Contributor

We need to externalize the following parameters for building the sap-connector projects,

  1. java-version
  2. java-distribution
  3. release-version
  4. next-dev-version.

While incorporating the required change, I found an issue of GitHub action while differentiating the input parameter between empty or not set in a reusable workflow and it is already an open issue at GitHub. Here is the reference - actions/runner#924

Hence, I needed to do it using condition construct.

Thank you!

…se-maven.yml in the alfresco-build-tools project
…se-maven.yml in the alfresco-build-tools project;adding new line at end
…se-maven.yml in the alfresco-build-tools project;perhaps fix the End of line issue
@SushmitaGoswami
Copy link
Contributor Author

Here is the link of the CI file of our project - https://github.com/Alfresco/sap-content-connector-parent/blob/feature/AFI-568/.github/workflows/ci.yml

This file is just for some testing at this moment. And also, for testing purpose, we created a local copy of the build-and-release-maven.yml which is referenced from the ci.yml at this moment.

Thanks in advance!

…se-maven.yml in the alfresco-build-tools project;removing the trailing space from step name
Copy link
Contributor

@dsibilio dsibilio left a comment

Choose a reason for hiding this comment

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

Since this action is being used in several repositories, it would be very important to have a link to an example workflow run where we don't pass any of the newly added parameters to verify that it still works as expected (installs the default Java version, computes the release and development version automatically) and this change won't break pre-existing workflows that get bumped to the latest version of this action.

.github/workflows/build-and-release-maven.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-release-maven.yml Outdated Show resolved Hide resolved
.github/workflows/build-and-release-maven.yml Outdated Show resolved Hide resolved
…se-maven.yml in the alfresco-build-tools project;incorporated review comments
…se-maven.yml in the alfresco-build-tools project;perhaps fix the CI/test
…se-maven.yml in the alfresco-build-tools project;perhaps fix the CI/test
@SushmitaGoswami
Copy link
Contributor Author

Updated the PR as per the review comments.

Following are the sample workflows which consume this build-and-release-maven workflow.

  1. Link to CI file (with custom value of the properties) - https://github.com/Alfresco/sap-content-connector-parent/blob/feature/AFI-1516/.github/workflows/release.yml
  2. Link to the CI file (with default value of the new properties) -

Please let me know if any additional change is required.

alxgomz
alxgomz previously approved these changes May 4, 2023
Copy link
Contributor

@alxgomz alxgomz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@gionn gionn left a comment

Choose a reason for hiding this comment

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

just some missing spaces, then LGTM

Comment on lines 70 to 71
java-version: ${{ inputs.java-version}}
java-distribution: ${{ inputs.java-distribution}}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
java-version: ${{ inputs.java-version}}
java-distribution: ${{ inputs.java-distribution}}
java-version: ${{ inputs.java-version }}
java-distribution: ${{ inputs.java-distribution }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dsibilio
dsibilio previously approved these changes May 4, 2023
…se-maven.yml in the alfresco-build-tools project;adding space
@SushmitaGoswami SushmitaGoswami dismissed stale reviews from dsibilio and alxgomz via 6efcbb4 May 4, 2023 08:26
@SushmitaGoswami SushmitaGoswami merged commit af9ca21 into master May 4, 2023
@SushmitaGoswami SushmitaGoswami deleted the feature/AFI-1520 branch May 4, 2023 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants