Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add macOS and Windows builds and update versions #11

Merged
merged 8 commits into from
May 13, 2022

Conversation

davidmfinol
Copy link
Member

  • Add macOS and Windows builds to workflow
  • Update versions of actions in workflow
  • Update Unity to version 2021.3.0f1

@@ -2,10 +2,12 @@ name: Unity Actions

on:
pull_request: {}
push: { branches: [main, master, develop] }
Copy link
Member

@webbertakken webbertakken Apr 23, 2022

Choose a reason for hiding this comment

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

If you have pull_request and push, then commits to PRs will trigger tests twice.

After merging something to main the tests should ideally run again, and in case a commit needs to be made to main directly it should also run the workflow. This is why push only included upstream branch names.

Not sure if you covered these cases in some other way somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this uses a personal license instead of a professional license, it should be fine if there are two instances of the workflow running at the same time, right? The pull_request workflow should use our credentials from our upstream repo, while the push workflow would use the user's credentials from their fork. Is there a potential problem with having the two workflow runs?

I've personally found it very useful to have the tests run on every push. It's been especially useful to confirm each commit in a feature branch.

Copy link
Member

Choose a reason for hiding this comment

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

I see. I don't know but a pull request from both local and a fork should just work as expected with the pull_request trigger. But I could be wrong?

Is there a potential problem with having the two workflow runs?

Yes actually we're hitting organisation-wide limits a lot with regards to runners, and the amount provided has become tighter as well a few months ago. When docker repo is building it's already stalling a lot for each commit, other repos don't currently have this by themselves, but they would block docker repo workflows.

However as the effect is probably quite minor I have no problem with moving forward with your changes if it solves a problem. Still trying to understand why push would be needed on all branches if a push to a PR already triggers on pull_request (as PR updated) afaik.

m_EditorVersion: 2020.1.7f1
m_EditorVersionWithRevision: 2020.1.7f1 (064ffcdb64ad)
m_EditorVersion: 2021.3.0f1
m_EditorVersionWithRevision: 2021.3.0f1 (6eacc8284459)
Copy link
Member

Choose a reason for hiding this comment

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

Just as a general note, ideally we wouldn't use the very first patch version in workflows that demonstrate the workflow works, as they're most prone to having bugs from Unity side. When people clone this project they're more likely to run into errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can wait a couple of months to give them time to fix any potential bugs, and then I'll update this PR to use v2021.3.5f1 or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Yea fair point. Feel free to move forward and we can make an issue to update it later.

@davidmfinol davidmfinol merged commit 9ab6fc0 into game-ci:main May 13, 2022
@davidmfinol davidmfinol deleted the v2021 branch May 13, 2022 18:04
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants