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

Add SPM validation in each PR #2181

Merged
merged 7 commits into from
Jun 27, 2024
Merged

Add SPM validation in each PR #2181

merged 7 commits into from
Jun 27, 2024

Conversation

diegojerezba
Copy link
Contributor

Proposed changes

This PR adds a new check in the pr-validation.yml. It consists in an integration of the changes of the current branch into the NativeAuth Sample App using a Swift Package.

These are the steps that are done:

  • Creates MSAL.zip of the xcframework.
    • It configures the URL and the checksum so the Sample App will use the changes we want to test.
  • Pushes it to a temporary branch.
  • Clones the NativeAuth Sample App repo, and configures it to download the Swift Package from the temporary branch.
  • Builds the Sample App.
  • Cleans up.

Note: I have checked the build.py script and I think it's different enough from what we are trying to do here, so I thought it is better to have two separate jobs, even if both scripts build the framework. I'm open to discussion on this.

The time it takes the PR-validation hasn't been incremented, as it runs in parallel and this job takes ~6-10 min.

The following tests have been done:

  • Test 1: (Pre)configure the sample app to use a new public method published in the current branch of MSAL.
  • Test 2: Make a breaking change in MSAL, the Sample App shouldn't work.
  • Test 3: If any build fails, the pipeline fails (and the temporary branch is deleted).
  • Test 4: If for any reason the checksum calculation doesn't work, the pipeline fails (and the temporary branch is deleted).

Type of change

  • Feature work
  • Bug fix
  • Documentation
  • Engineering change
  • [x ] Test
  • Logging/Telemetry

Risk

  • High – Errors could cause MAJOR regression of many scenarios. (Example: new large features or high level infrastructure changes)
  • Medium – Errors could cause regression of 1 or more scenarios. (Example: somewhat complex bug fixes, small new features)
  • Small – No issues are expected. (Example: Very small bug fixes, string changes, or configuration settings changes)

@diegojerezba diegojerezba requested a review from a team as a code owner May 29, 2024 17:57
spm-integration-test.sh Outdated Show resolved Hide resolved
@diegojerezba diegojerezba force-pushed the diegoje/spm-integration-ci branch 2 times, most recently from f54b34f to 71bc558 Compare June 13, 2024 13:55
@nilo-ms nilo-ms added the native-auth Code related to native authentication label Jun 13, 2024
@diegojerezba diegojerezba requested review from ameyapat and removed request for antonioalwan June 14, 2024 09:32
targetType: 'inline'
script: |
sh spm-integration-test.sh "${BRANCH_NAME}"
continueOnError: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be continueOnError: false? How will errors during build with this branch's MSAL swift package be caught and cause the pipeline to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, thanks for the catch.

continueOnError: true

- task: Bash@3
displayName: Cleanup
Copy link
Contributor

Choose a reason for hiding this comment

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

Add condition: always() for this task so that it always runs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, thank you.

inputs:
targetType: 'inline'
script: |
UUID_LOCAL=$(uuidgen)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: We don't need to create a UUID guid if the branch is going to be deleted in the last step anyways. We can create and delete a branch with the same name

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

@ameyapat
Copy link
Contributor

Added a few comments. Otherwise, lgtm

Copy link
Contributor

@ameyapat ameyapat left a comment

Choose a reason for hiding this comment

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

I'd suggest to manually run the pipeline on this branch as a sanity check

@diegojerezba
Copy link
Contributor Author

I'd suggest to manually run the pipeline on this branch as a sanity check

Done: https://identitydivision.visualstudio.com/IDDP/_build/results?buildId=1320628&view=results

@diegojerezba diegojerezba merged commit 41def13 into dev Jun 27, 2024
10 checks passed
@diegojerezba diegojerezba deleted the diegoje/spm-integration-ci branch June 27, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
native-auth Code related to native authentication
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants