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

Update Fusion's CI Workflow #223

Merged
merged 4 commits into from
Feb 6, 2023
Merged

Update Fusion's CI Workflow #223

merged 4 commits into from
Feb 6, 2023

Conversation

krypt102
Copy link
Contributor

@krypt102 krypt102 commented Feb 5, 2023

Fixes #208
This PR updates selene to the latest version (as suggested in #208).
I've removed the Format and Unit Tests jobs as they don't seem useful anymore, however if we still need them then I can re-add them back.

@krypt102 krypt102 marked this pull request as ready for review February 6, 2023 04:13
.github/workflows/mkdocs-deploy.yml Outdated Show resolved Hide resolved
@dphfox dphfox added ready to work on Enhancements/changes ready to be made targeting: meta About the Fusion project/maintenance/repo labels Feb 6, 2023
@dphfox dphfox merged commit 0a439ec into dphfox:main Feb 6, 2023
@krypt102 krypt102 deleted the ci-update branch February 6, 2023 21:40
@Dionysusnu
Copy link
Contributor

Dionysusnu commented Feb 7, 2023

Removing the workflows entirely was unnecessary - You could've just disabled them semi-temporarily, especially considering the intent to move the test CI outside of Roblox.

Missing the trailing newline in the file is a bit annoying for future git diffs.

Removing the pull request workflow trigger is also quite annoying because it makes the CI not act until it's already too late when a failing check would happen.

@krypt102
Copy link
Contributor Author

krypt102 commented Feb 8, 2023

Removing the pull request workflow trigger is also quite annoying because it makes the CI not act until it's already too late when a failing check would happen.

This was only done because iirc Elttob suggested to do it on the issue so we don't have two of the same action running.

In hindsight I probably should have just commented out the other 2 workflows, however from what was said in the issue itself they didn't seem useful to include. Would you mind elaborating on what "taking the CI out of Roblox" would mean?

@krypt102
Copy link
Contributor Author

krypt102 commented Feb 8, 2023

Missing the trailing newline in the file is a bit annoying for future git diffs.

Oh whoops! Didn't see this in the diff, will keep in mind!

@Dionysusnu
Copy link
Contributor

Removing the pull request workflow trigger is also quite annoying because it makes the CI not act until it's already too late when a failing check would happen.

This was only done because iirc Elttob suggested to do it on the issue so we don't have two of the same action running.

That was me, but I suggested it differently. The PR trigger should be kept, alongside adding the main-branch-only condition for the push trigger.

In hindsight I probably should have just commented out the other 2 workflows, however from what was said in the issue itself they didn't seem useful to include. Would you mind elaborating on what "taking the CI out of Roblox" would mean?

Let's make a new issue for further discussion on it, but Elttob mentioned the intention to separate the unit testing into two separate parts: One that deals with Lua only, and one that includes Roblox API semantics. Once we do that, the former can then be CI tested in GitHub once again.

@krypt102
Copy link
Contributor Author

Let's make a new issue for further discussion on it, but Elttob mentioned the intention to separate the unit testing into two separate parts

Okay! Once you've made it please mention me :D

@dphfox
Copy link
Owner

dphfox commented Feb 12, 2023

Removing the pull request workflow trigger is also quite annoying because it makes the CI not act until it's already too late when a failing check would happen.

This was only done because iirc Elttob suggested to do it on the issue so we don't have two of the same action running.

That was me, but I suggested it differently. The PR trigger should be kept, alongside adding the main-branch-only condition for the push trigger.

In hindsight I probably should have just commented out the other 2 workflows, however from what was said in the issue itself they didn't seem useful to include. Would you mind elaborating on what "taking the CI out of Roblox" would mean?

Let's make a new issue for further discussion on it, but Elttob mentioned the intention to separate the unit testing into two separate parts: One that deals with Lua only, and one that includes Roblox API semantics. Once we do that, the former can then be CI tested in GitHub once again.

To be clear btw, I didn't mean to imply that I actively intended to do this. It was more of a passing idea that we might consider in the future.

I've already been thinking about segregating Fusion along Roblox API lines so we could perhaps open up the core to other Luau environments (maybe perhaps even Lua 5.1, we'll see). None of this is an explicit intention though, more prospective stuff in my head. I'll make an issue about it if I want to present any ideas to you guys :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to work on Enhancements/changes ready to be made targeting: meta About the Fusion project/maintenance/repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Fusion's CI on GitHub
3 participants