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

GitHub: Added build CI #13620

Merged
merged 87 commits into from
Nov 10, 2023
Merged

Conversation

0x5bfa
Copy link
Member

@0x5bfa 0x5bfa commented Oct 31, 2023

Summary

We are finally going to make build & test CI on the GitHub Actions to gain better integrity. This enables us to configure and manage the CI easily, and have better access to the package artifacts.

We had some difficulties below (wrote down to view quickly what we've overcome)

  • Not being able to run the interaction tests on GitHub Actions
    • Fixed by adding proper app id, though it is still unclear that we were able to run that tests on Azure DevOps properly.
  • Not being able to add a comment on PR that triggered CI
    • Will have fixed by using 'pull_request_target', which gives us more permissions nonethless makes the workflow more secure. If you'd like to know the differences between them, visit GitHub docs.

Task Checklist

  • Added build job
  • Added test job
  • Added creator of a comment on PR
  • Clean up
  • Apply requested changes

Known Issues

N/A

PR Checklist

  • Closes: N/A
  • Approval: Have discussed with and got approval from the team1
  • Tests: Tested accessibility on the local end
  • Deployment: Deployed the app on the local end
  • Localization: Modified en-US string resources2
  • Co-authors: @chingucoding

Steps To Validate Changes

  1. Create a commit on your branch on your files-community fork.
  2. Add a PR
  3. See CI run building and testing

Screenshots

N/A

Footnotes

  1. If the request hasn't been agreed by the team, this work might be rejected. Make sure that you get approval from the team before opening any PR related the request.

  2. If you removed any en-US string resources, make sure that there are no references of those resources. When you add a new en-US string resources, do not make any changes on other languages' resources; Crowdin will do that, instead.

builds/azure-pipelines.yml Outdated Show resolved Hide resolved
@heftymouse
Copy link
Contributor

heftymouse commented Nov 8, 2023

It'd be good to cache NuGet packages by setting <RestorePackagesWithLockFile>true</RestorePackagesWithLockFile> and a step like this:

- uses: actions/cache@v3
  id: nuget-cache
  with:
    path: ${{ env.NUGET_PACKAGES }}
    key: ${{ runner.os }}-${{ hashFiles('**/packages.lock.json') }}

- name: Restore NuGet
    if: steps.nuget-cache.outputs.cache-hit != 'true'
    run: ...

Note: I haven't tested this at all

@0x5bfa
Copy link
Member Author

0x5bfa commented Nov 8, 2023

@heftymouse You mean this save us time about 3 minis in running Restore NuGet?

If so, that's nice and I'm going to do so!

I'd rather wait to add this feature here now because I'm not sure I can run restoring when one of the cached dependencies updated, which is very important.

@heftymouse
Copy link
Contributor

I'm not sure I can run restoring when one of the cached dependencies updated, which is very important.

That's what the lockfile and the key in the action are for, it'll do a full restore when any of the dependencies are updated

I'm realising now that a better approach would be to just restore every time and cache the result, so if any of the dependencies change it just has to restore the changes

@0x5bfa
Copy link
Member Author

0x5bfa commented Nov 8, 2023

Is it better behavior that CI restores from nuget.org (takes 3 min) and cache result always on the main branch (on: push) and CI restores only from cached pool to make the CI on the PR (on: pull_request) as fast as and as reliable as possible?

@heftymouse
Copy link
Contributor

Is it better behavior that CI restores from nuget.org (takes 3 min) and cache result always on the main branch (on: push) and CI restores only from cached pool to make the CI on the PR (on: pull_request) as fast as and as reliable as possible?

Sure, that works

@0x5bfa
Copy link
Member Author

0x5bfa commented Nov 8, 2023

For the time being, I'd rather keep as is.

@yaira2 Ready for review.

@yaira2 yaira2 changed the title GitHub: Add build CI GitHub: Added build CI Nov 8, 2023
yaira2
yaira2 previously approved these changes Nov 8, 2023
@0x5bfa
Copy link
Member Author

0x5bfa commented Nov 9, 2023

.NET test is failing now.

@0x5bfa
Copy link
Member Author

0x5bfa commented Nov 9, 2023

Ready for merging

@yaira2 yaira2 added ready to merge Pull requests that are approved and ready to merge and removed needs - code review labels Nov 10, 2023
@yaira2 yaira2 merged commit d23b0b3 into files-community:main Nov 10, 2023
6 checks passed
@0x5bfa 0x5bfa deleted the 5bfa/Add-DevCIActions branch November 10, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Pull requests that are approved and ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants