-
-
Notifications
You must be signed in to change notification settings - Fork 28
Add release workflow #40
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
Conversation
|
Welp upload went through but @asherkin you'll have to tell me where they ended up lol. It certainly isn't at builds.limetech.io/?test nor builds.limetech.io/?p=accelerator |
|
@sapphonie do not merge this until situation above is solved, because I gotta add few more files to the workflow but I need to confirm upload is working first. |
|
It made it! Not displayed on the page as the filename doesn't match the expected format - no incrementing ID for sorting builds with the same version number. I can get the regex it uses later, but matching the format of the existing git ones is probably easiest. EDIT: Can make that optional, but think it's needed. |
|
Ah yes! I forgot the git rev count tag that's where it failed, fixing that right now. |
|
I'm guessing once added properly this'll never be uploading from a PR, but FWIW there's a |
|
Okay well there's a mistake I zip both platform as "window" |
|
This should be it ? Unsure why the builds still don't show up. |
Ah this comment got eaten while I was replying to the PR, my apologies I didn't mean to ignore. Yes I plan on restricting the upload script to push on master directly, but I'll also add the branch param just in case :) |
|
I see all the recent ones with |
Oh right, that's where they went oops I forgot! I'll add the branch param then |
|
New uploads succeeded but don't show up I assume that's because of the branch name check, which means everything's good ? I'm going to switch off release from PR workflow. |
|
That's "worked", but the branch was apparently just called "40"? |
Okay yeah that's wrong lol, github actions env variables are hard to understand. Time to investigate |
|
Turns out it was actually "40/merge", which has rather confused it - not what's expected either way though! I'll fix it up at some point to turn slashes into hyphens or something though, just in case it is actually expected here. |
|
@Kenzzer Actually, I suspect that is the correct branch name, and what we'd want in the case of real test builds from PRs, so this probably is good to go! |
|
For paranoia sake I'll add quotation marks around each query param just to ensure no weird parsing can happen. We should be getting "release_ci" though, I suspect it replaced it with "40/merge" because of this PR. I think if I tinker a bit with the triggers I can get "release_ci" to show up once more. |
Oh well same thoughts haha. I'll run one more quick test with the release CI if that's okay |
|
We should have We must basically be careful when testing release.yml, we must edit the |
|
will this not push random untrusted prs to limetech as written? |
Thankfully no, secrets are disabled across forks even if somebody gets the bright idea to disable all the workflows/write their own to print secrets github won't allow it, a maintainer will have to approve the workflows. Secrets are only shared with branches made on the same repo, so basically only you and I can do something nasty with the upload password. |
|
nm; also ty for the clarification regarding secrets, makes sense |
|
Do not worry if you see 9 jobs that's normal. There's 4 jobs (Release) running for this branch There's 5 jobs (Pull request) running for all pull requests to So if @asherkin greenlights this, we can merge without further modifications to the workflow files. |
|
Given the logs, my suspicions were correct One simply has to be careful in how the workflow file is invoked; i.e must be limited to (push) |
|
The "cool" thing in the way I made this is that we can add any amount of branches we want to the release workflow. This will make any future dev easier, rather than having "if"(s) everywhere in the yml file and getting confused. That also takes out the "release logic" out of the "build logic" keeping every yml file straight forward. |
|
Getting |
|
Hmm gonna try %22, if it solves nothing then quotation less it is |
You don't want that - encoding them would guarantee them getting to the server, and they already are! |
|
okay yay 💜 |
|
When I'm home and remember (lol), I'll swap the test password for the real one. |
|
👀 ? |
|
Hopefully this is just a sorting issue and not a bug. I just merged the PR #41 and created commit 3ed1266 and the subsequent test release build isn't at the top builds.limetech.io. Instead |
|
Oh... I think I see the problem, this is probably linked to the revision tag. The wrong latest build is tagged with Okay nevermind then, this is just an unfortunate side effect of when we tested the CI. This issue won't appear on the real build page. |
It has been done. |
No description provided.