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

Test: Create minimal unit test for Maven jobs #175

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zxiiro
Copy link
Member

@zxiiro zxiiro commented Jul 24, 2024

This change creates a minimal pom.xml for unit testing of Maven jobs and creates a test job for the Maven Verify workflow. The intent here is to create a small unit test that can quickly validate Maven workflows and catch potential issues that maybe missed by a linter.

@zxiiro zxiiro requested a review from a team July 24, 2024 16:18
@zxiiro
Copy link
Member Author

zxiiro commented Jul 24, 2024

This change depends on #174 being merged as it requires to fix to the input parameter to choose an alternative pom.xml file.

@zxiiro
Copy link
Member Author

zxiiro commented Jul 24, 2024

Looks like it also needs a GLOBAL_SETTINGS variable defined.

echo '' > global-settings.xml

It doesn't need to have any real data. Something like this should be sufficient:

<settings xmlns="http://maven.apache.org/SETTINGS/1.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/SETTINGS/1.0.0 https://maven.apache.org/xsd/settings-1.0.0.xsd">
  <localRepository/>
  <interactiveMode/>
  <offline/>
  <pluginGroups/>
  <servers/>
  <mirrors/>
  <proxies/>
  <profiles/>
  <activeProfiles/>
</settings>

@zxiiro
Copy link
Member Author

zxiiro commented Jul 24, 2024

Alright this is ready to go but the unit test won't pass until the GLOBAL_SETTINGS variable is set as mentioned in my previous comment. Unfortunately I don't have the permissions to add it myself so someone in @lfit/release-engineering will have to add it and retest before this can be merged.

@zxiiro
Copy link
Member Author

zxiiro commented Jul 25, 2024

To make the code cleaner it proposed #176 and removed the unused parameters that I was putting dummy values in for.

tykeal
tykeal previously approved these changes Jul 29, 2024
Copy link
Member

@tykeal tykeal left a comment

Choose a reason for hiding this comment

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

This looks ok to me, but the job doesn't appear to be picking up the newly created GLOBAL_SETTINGS var that I added per your earlier statements.

@zxiiro
Copy link
Member Author

zxiiro commented Jul 29, 2024

This looks ok to me, but the job doesn't appear to be picking up the newly created GLOBAL_SETTINGS var that I added per your earlier statements.

It should be pulling from the repo's variables based on this code here. Calling {{ vars.GLOBAL_SETTINGS }}.

Is there any security configuration in this repo preventing forked repos from fetching variables from the upstream repo?

@zxiiro
Copy link
Member Author

zxiiro commented Jul 29, 2024

Oh I was reading the docs for repo variable configuration and it says this:

Anyone with collaborator access to this repository can use these secrets and variables for actions. They are not passed to workflows that are triggered by a pull request from a fork.

ref: https://github.com/lfit/releng-reusable-workflows/settings/variables/actions

Yep, confirmed looks like variables definitely are not passed to forked repos. :(

https://github.com/orgs/community/discussions/44322

I'll see if we can code around this somehow.

@zxiiro
Copy link
Member Author

zxiiro commented Jul 30, 2024

Success! I had to make a small change to allow an input MVN_GLOBAL_SETTINGS in order to allow the unit test to work from forked repos by hardcoding a minimal global settings file. The current behaviour continues to be the default behaviour even with this change so real jobs will still get ${{ var.GLOBAL_SETTINGS }} as expected.

Since var.GLOBAL_SETTINGS isn't passed down to forked repos though this means this is still a problem for any projects that are using GitHub to accept contributions rather than Gerrit. Which makes me think likely all of our maven jobs likely don't work projects using GitHub directly that want to accept contributions from forked repos.

This change creates a minimal pom.xml for unit testing of Maven jobs
and creates a test job for the Maven Verify workflow. The intent here
is to create a small unit test that can quickly validate Maven
workflows and catch potential issues that maybe missed by a linter.

This change also modifies the compose-maven-verify job to accept a new
parameter MVN_GLOBAL_SETTINGS which is optional and defaults the the
current behaviour while allowing it to be overrided to provide an
alternative global-settings which is necessary for this unit test to
work on PRs from forked repos.

Signed-off-by: Thanh Ha <[email protected]>
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.

2 participants