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

Allow to set the time waited between two cargo publishs #257

Merged
merged 5 commits into from
Jun 16, 2021

Conversation

sebschmi
Copy link
Contributor

This adds a command line parameter --publish-grace-sleep that defines how long we wait between two invocations of cargo publish. The default value is 5.

@epage
Copy link
Collaborator

epage commented Jun 3, 2021

Sorry no one has responded on this. This could actually help in #218.

There is a part of me that would prefer for this to be an env variable, like RUST_BACKTRACE. I think part of it stems from this being a bit of a stopgap feature and not wanting to bake it into the CLI and clutter up the help.

@sunng87 or @sebschmi any thoughts on that?

@sebschmi
Copy link
Contributor Author

sebschmi commented Jun 3, 2021

I would be totally fine with an env variable as well.

Has btw anyone made an issue at https://github.com/rust-lang/crates.io/issues about some API feature to poll if a crate is ready?

@epage
Copy link
Collaborator

epage commented Jun 3, 2021

Has btw anyone made an issue at https://github.com/rust-lang/crates.io/issues about some API feature to poll if a crate is ready?

My understanding is that git is the API; we've just not been able to determine what we are doing differently than cargo when it determines a crate is available.

@sebschmi
Copy link
Contributor Author

sebschmi commented Jun 3, 2021

I made an issue at crates.io now anyways. I think it would be useful to have a simple way to automate crate publishing without complicated hacks.

@sunng87
Copy link
Collaborator

sunng87 commented Jun 3, 2021

@epage Do you have any suggestion about the criteria to classify the location of configuration, among cli options, toml files or environment variables?

I think I have a little sense with the difference between cli options and toml files. But for environment variable, I can't tell it from cli options.

@sunng87
Copy link
Collaborator

sunng87 commented Jun 3, 2021

@sebschmi you might want to format the code to pass our rustfmt check.

@epage
Copy link
Collaborator

epage commented Jun 4, 2021

@epage Do you have any suggestion about the criteria to classify the location of configuration, among cli options, toml files or environment variables?

I think I have a little sense with the difference between cli options and toml files. But for environment variable, I can't tell it from cli options.

Im honestly going off of gut feel here and can be completely wrong :)

I think typos is where I;ve started to most formalize my thoughts on CLI inputs:

  • User / integration relevant settings (e.g. output format): I'm using the CLI at the moment because there are few and defaults are good enough. I might add user config later.
  • Repo relevant settings these live in a config file in the repo
  • Iterative repo relevant settings: I exposed these both on the CLI and in the repo's config file (e.g. excluding files from being analyzed)

So far, I only use env variables to comply with the various CLI colored output standards.

In this case, the wait time doesn't really fit into any of the categories. This is a workaround for another bug and there isn't anything about this that is repo -specific. For user-relevant, it sort of is just by how much the user can tolerate failure vs delays. For integration-relevant, it sort of is to help us with #218 where there is another bug/limitation in our wait logic.

So based on my earlier classifications. this most closely aligns with how I've been using CLI args / user-config. The reason I'm considering other approaches, like modeling after some crate processes, like RUST_BACKTRACE, is because this is a workaround. cargo-release does not have a user config file and it already has a sizeable CLI "API" for people to navigate. I don't see this being something that should have raised visibility and being as formally specified. This led me to the thought of env variables.

@sebschmi
Copy link
Contributor Author

sebschmi commented Jun 4, 2021

@sunng87 I will keep out of the discussion of this being a CLI argument or env variable. But as soon as you come to an agreement, I will make the necessary changes, also to make it pass the CI checks.

@sunng87
Copy link
Collaborator

sunng87 commented Jun 4, 2021

because this is a workaround

@epage sounds good to me. I agree with the idea of putting it into environment variables. cc @sebschmi

@sebschmi
Copy link
Contributor Author

sebschmi commented Jun 7, 2021

I changed the code, and kept the impact of the env variable minimal: I simply directly query it at the // HACK comment.

I did not document this feature, if you want to I can do it.

@sunng87 sunng87 merged commit d08515b into crate-ci:master Jun 16, 2021
@sunng87
Copy link
Collaborator

sunng87 commented Jun 16, 2021

@sebschmi sorry for late response. I will update document for you.

@sebschmi
Copy link
Contributor Author

No worries, thank you very much for merging!

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.

3 participants