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

fix(crates): Resume workspace publish #392

Merged
merged 2 commits into from
May 11, 2022
Merged

Conversation

jan-auer
Copy link
Member

@jan-auer jan-auer commented May 10, 2022

The crates target is not able to resume a workspace publish because already published crates cannot be republished. This means a partially successful publish cannot be retried.

To work around this, the crates target can skip publishing a crate if it already exists in this version. The downside of this is that we would silence errors when publishing the wrong version.

See getsentry/publish#1070 (comment)

@jan-auer jan-auer requested a review from kamilogorek May 10, 2022 08:44
@jan-auer jan-auer self-assigned this May 10, 2022
@BYK
Copy link
Member

BYK commented May 10, 2022

I'd argue a better way to handle this is to have multiple crates targets per package.

@BYK
Copy link
Member

BYK commented May 10, 2022

Or at least not make this behavior the default and hide behind a flag.

@jan-auer
Copy link
Member Author

jan-auer commented May 10, 2022

Having multiple crates targets is pretty much not an option here. In workspaces, you frequently have dozens of crates which you would then have to order topological by their dependency graph. That's too much manual work every time, unfortunately.

We could also add this as a flag, @chadwhitacre is there an easy way to pass a "this is a retry" flag to craft already?

@BYK
Copy link
Member

BYK commented May 10, 2022

Having multiple crates targets is pretty much not an option here. In workspaces, you frequently have dozens of crates which you would then have to order topological by their dependency graph. That's too much manual work every time, unfortunately.

I never said "manual" 😁 I think we can expand the publish state file format to have this extra metadata.

@jan-auer
Copy link
Member Author

I see, yes that makes much more sense. If the crates target could remember the crates it already published, it could just pick up where it left off. This PR here is more meant as a quick fix, although I'm happy to wait a little longer for something proper.

@chadwhitacre
Copy link
Member

I say we merge here and reticket @BYK's idea for running this through the state file.

@chadwhitacre
Copy link
Member

Maybe with an additional comment linking to the new ticket.

Copy link
Contributor

@kamilogorek kamilogorek left a comment

Choose a reason for hiding this comment

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

👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants