Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@Xanewok
Copy link
Contributor

@Xanewok Xanewok commented Jul 22, 2021

...rather than doing it only on select tags. This should publish only versions that are not published yet.

Related #7634

cc @gnunicorn @TriplEight

@Xanewok Xanewok added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 22, 2021
@Xanewok Xanewok marked this pull request as ready for review July 22, 2021 14:19
@Xanewok Xanewok requested a review from a team as a code owner July 22, 2021 14:19
<<: *docker-env
<<: *test-refs-no-trigger
script:
- cargo install cargo-unleash ${CARGO_UNLEASH_INSTALL_PARAMS}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it mature enough to be installed into CI image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably - until it's not v1 I'd prefer not to; the time savings is miniscule I'd say comparing to the actual checking done by the tool later on

Copy link
Contributor

Choose a reason for hiding this comment

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

not on every commit tho

unleash-check:
stage: test
<<: *docker-env
<<: *test-refs-no-trigger
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really want to publish before the tests are green?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the unleash-check - I think it should be run early/at the same time as tests; only publishing should be done after every test is green

Copy link
Contributor

Choose a reason for hiding this comment

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

In my view (I actually do not know what does it check, Check whether crates can be packaged in the docs is probably not enough) it's an overkill to run these checks on every commit. We should run them before the release target, so maybe on master, or even nightly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand this correctly, what you propose may lead to the PR checks being green at first but only discovered to be red once merged. If that's the case, then won't be much incentive to fix this once a PR is merged and it'll just hang around being red until someone disables it again - I'd rather we catch that early.

If this is deemed to expensive to run on every commit, we might want to run this as an extra step for bot merge to not merge a given PR until we know for sure that this check is green.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is expensive, the job takes 30 minutes. Any possible way to combine it with something to reuse the cache?

Ideally, it will run on the pre-merge pipeline and a merge-train, but they are not ready yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to either check only if certain files are changed or versions got bumped?

publish-to-crates-io:
stage: publish
needs:
- job: unleash-check
Copy link
Contributor

Choose a reason for hiding this comment

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

Either you should list the jobs necessary to be green in order to start this job, because this way it starts right after unleash-check without waiting anything else. Or remove this needs and the stage won't start if the previous jobs are not successful. I prefer the latter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we somehow make it so that it runs at publish but also make sure that unleash-check is never allow_failure: true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By publish I mean only after everything else completes from the test stage and so on

Copy link
Contributor

Choose a reason for hiding this comment

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

We can run unleash check on the build stage, so after all the code tests pass, and unleash em-dragons on the publish phase after everything's proven to be buildable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move this to the build stage if you want me to, running parallel to the tests is also good (at least wrt developer feedback latency, maybe not so much wrt CI builder overhead 😀 )

Copy link
Contributor

Choose a reason for hiding this comment

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

it's worth it, the test stage already brings a lot of concurrent jobs. The longest job in the test stage is test-linux-stable , which takes 36 minutes. This one takes ~32. The longest on the build stage is check-polkadot-companion-build , so unless we make use of caching (or simplify it somehow) it will severely contribute to the pipelines concurrency.

@TriplEight TriplEight self-requested a review July 22, 2021 14:53
@TriplEight
Copy link
Contributor

@s3krit what's your take on releasing on every commit to master? That's something even beyond nightly releases in my view.

publish-to-crates-io:
stage: publish
needs:
- job: unleash-check
Copy link
Contributor

Choose a reason for hiding this comment

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

We can run unleash check on the build stage, so after all the code tests pass, and unleash em-dragons on the publish phase after everything's proven to be buildable.

# cycle and cannot proceed with publishing/checking
- cargo unleash de-dev-deps
- cargo unleash em-dragons --no-check --owner github:paritytech:core-devs --dry-run
allow_failure: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this may be removed as soon as it will publish stably.

@bkchr
Copy link
Member

bkchr commented Jul 23, 2021

This means we can finally increase a crate version and it will be published?

@s3krit
Copy link
Contributor

s3krit commented Jul 23, 2021

@s3krit what's your take on releasing on every commit to master? That's something even beyond nightly releases in my view.

My general view would be that it might be better for developers to have the power themselves to bump versions and only then would it get pushed to crates.io. I.e., rather than us using CI to detect whether or not we need to push to crates.io or whatever, we simply let the much smarter developers decide whether they want to release a new version of a crate (and subsequent dependencies).

That being said, if we're not really doing much in the way of additional testing/QA once something's been committed to master, I'm not really sure what the drawback is. If we're happy for something to be merged with master, due to the way we currently use substrate and builders use substrate, we're basically saying 'this is OK to use' (as I understand it). And pushing to crates.io just makes it easier for developers. Of course, I could be entirely wrong here.

Copy link
Contributor

@TriplEight TriplEight left a comment

Choose a reason for hiding this comment

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

Right, let's publish just on version bumps.

@gnunicorn
Copy link
Contributor

gnunicorn commented Jul 29, 2021

This means we can finally increase a crate version and it will be published?

Yes.

We should run cargo unleash em-dragons only on the version bumps, please return it -- @TriplEight

That is inconsistent with the requirement from @bkchr just stated above. Why should this not be run unless a specific tag is set?

@s3krit what's your take on releasing on every commit to master? That's something even beyond nightly releases in my view.

This discussing is irrelevant, as this is not what is happening here.


To clarify: cargo unleash em-dragons first syncs the latest crates.io versions, then checks the local tree against them. Only the crates that locally have a version not found on crates.io (and don't have a publish = false flag in their Cargo.toml) will be attempted to be released. If the PR didn't change any version (for the majority of PRs) – no matter what others changes have occurred – this does exactly nothing, states that all is fine and quits.

@TriplEight
Copy link
Contributor

Ok, this makes sense then. Thanks @gnunicorn . Then it should run on master.

@Xanewok
Copy link
Contributor Author

Xanewok commented Aug 3, 2021

@ascjones do we need to wait until #8615 is merged?

@ascjones
Copy link
Contributor

ascjones commented Aug 3, 2021

@ascjones do we need to wait until #8615 is merged?

If this PR will release frame-metadata from this repo, then yes.

If it won't release that crate then should be good to go.

@Xanewok
Copy link
Contributor Author

Xanewok commented Aug 3, 2021

AIUI an alternative would be to proceed with publishing the 14.0.0-dev here, and yank it once #8615 is merged (which also would properly migrate to an "actual" v14, so to speak); I'm not sure how much time #8615 needs.

@ascjones
Copy link
Contributor

ascjones commented Aug 3, 2021

I'm not sure how much time #8615 needs.

It's waiting for another review, but not sure how long that will take tbh.

an alternative would be to proceed with publishing the 14.0.0-dev here

That might work, though I would need to change the deps on my PRs to use the git dependency which would break the unleash CI. But I think that has changed now to be non-mandatory.

unleash-check:
stage: test
<<: *docker-env
<<: *test-refs-no-trigger
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's better to either check only if certain files are changed or versions got bumped?

Co-authored-by: Denis Pisarev <[email protected]>
@stale
Copy link

stale bot commented Sep 2, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 2, 2021
stage: publish
needs:
- job: unleash-check
<<: *docker-env
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<<: *docker-env
<<: *docker-env
<<: *build-refs

unleash-check:
stage: test
<<: *docker-env
<<: *test-refs-no-trigger
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<<: *test-refs-no-trigger
<<: *build-refs

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 13, 2021
@TriplEight
Copy link
Contributor

cc #9705

@stale
Copy link

stale bot commented Oct 13, 2021

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Oct 13, 2021
@bkchr bkchr closed this Oct 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants