-
Notifications
You must be signed in to change notification settings - Fork 90
few cargo updates #337
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
few cargo updates #337
Conversation
Are you still using minify? |
Perfect, thanks! Yes, as far as I can remember, minify is still being used. Just re-running that failing check and then good to merge. |
That test has failed again, @afidegnum could you run locally and investigate the preload test? |
I was able to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can re-enable PR CI runs, I'm happy to merge this once preload tests are passing (or once there's a concrete reason they aren't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why have you disabled pull request CI runs? It's important to have these so we can make sure PRs work in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that from the repo I forked not the push committed in order to properly identify the bug. once done, I will submit the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay, it's just that CI runs on pull requests have been disabled in the commits here. Once that's fixed I'm happy to merge this.
HI, I have been able to fix the issues, can you pls accept the pull request? |
I can't see the changes you requested, can you please give details? |
Thanks! Will have a look at this by tomorrow. |
have you checked? is it ok? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So sorry for the delay with this, life very much happened! One little change to make sure the CI script still works, and then this is good to merge. Preload tests seem to be passing now, thanks!
Co-authored-by: Sam Brew <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks!
Just re-running the failing capsules test, should be fine. Then will merge. |
Thanks, I'm investigating on the failed test and working on the sycamore updates too. |
Please add the following cargo updates while I work on others.