Skip to content

add maintainer docs and Cargo.lock #113

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

Merged
merged 10 commits into from
Oct 14, 2021
Merged

add maintainer docs and Cargo.lock #113

merged 10 commits into from
Oct 14, 2021

Conversation

davepacheco
Copy link
Collaborator

@davepacheco davepacheco commented Jun 1, 2021

This PR:

  • adds docs for maintainers that cover publishing a new release and our use of Dependabot
  • adds Cargo.lock, as recommended by @cbiffle (and documents why we do it)
  • updates CI to enforce that Cargo.lock is correct by building with --locked.

If the Cargo.lock check-in is controversial, I can do that in a separate PR. It's here because I thought I'd already done it, so I documented it, and then realized that I hadn't actually done it yet, and it didn't seem worth splitting out.

CC @ahl @smklein. I think there are probably too many words here, but I wanted to err on the side of more docs rather than less.

@davepacheco davepacheco assigned davepacheco, ahl and smklein and unassigned davepacheco Jun 2, 2021
@davepacheco davepacheco marked this pull request as ready for review June 2, 2021 03:18
MAINTAINERS.adoc Outdated

For applications (such as Oxide's internal Omicron project), we point Dependabot at the root of the https://doc.rust-lang.org/cargo/reference/workspaces.html[Cargo workspace] (the root of the repository). This causes Dependabot to find Cargo.lock, treat the package as an application, and update Cargo.lock and the Cargo.toml files for all packages in the workspace.

We do not do this in Dropshot. We _do_ <<_checking_in_cargo_lock_file,check in a Cargo.lock file>>, but we point Dependabot at the individual packages in the Dropshot workspace. Dependabot does not find the Cargo.lock file and so treats Dropshot as a library, only updating Cargo.toml when _necessary_ to accommodate a new dependency.
Copy link
Contributor

Choose a reason for hiding this comment

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

Earlier we made the assertion the following regarding Lockfiles:

Since the CI tests always run against a particular commit, this allows us to know exactly which dependencies were used for a particular CI run. It also communicates to consumers what we've tested.

If dependabot can update Cargo.toml files without updating Cargo.lock files, hasn't this assertion been broken? If dependabot updates a rev, but Cargo.lock is left out of date, then won't CI runs be inconsistent?

If we need to add the caveat that "Cargo.lock shows what dependencies are used, unless dependabot changed something, in which case it might be out-of-date", IMO the value of keeping a lockfile around drops significantly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's a good point. Better options here are probably:

  • don't check in Cargo.lock
  • point dependabot at the whole workspace and treat it like an application
  • point dependabot at the whole workspace and try to configure it to treat it like a library. This seems best to me, but I'm not super optimistic based on the quality of the docs and the time I spent reverse engineering this last time.

Copy link
Collaborator Author

@davepacheco davepacheco Jun 4, 2021

Choose a reason for hiding this comment

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

It turns out that the docs are actually correct:

The property '#/updates/0/versioning-strategy' value "increase-if-necessary" did not match one of the following values: lockfile-only, auto

From looking at the source a few weeks ago, the "auto" option decides whether it's an app or a library based on the presence of Cargo.lock (Cargo.lock present => application). So if we use "auto", dropshot will be treated like an application, which means dependabot will be somewhat aggressive about updating dependencies. If we chose "lockfile-only", I believe it would bump Cargo.lock as compatible updates to dependencies were published, but never update Cargo.toml (which largely defeats the point).

I think I'd like to try letting dependabot treat dropshot like an application. The downside is that if we have a dep version like 1.2.3, and 1.3 comes out, it will update Cargo.toml to specify 1.3, even though we might be fine with 1.2.3 and 1.3, so we're imposing additional constraints on our consumers. I'm hopeful that we can avoid this problem by avoiding such specific dependencies in Cargo.toml unless we really need them. That is: if we leave our dep at 1.2, and 1.3 is compatible with that, dependabot won't update Cargo.toml. (I'd expect it to update Cargo.lock, and that's fine and probably useful.)

I've done this in 6f9adbd.

Comment on lines +83 to +99
+
Now, run `cargo release` for real (i.e. no `--dry-run`), but without publishing to crates.io or pushing the git tags:
+
[source,text]
----
$ cargo release --skip-push --skip-publish --prev-tag-name=PREV_RELEASE_TAG -vv minor
----
+
Inspect the resulting commits and the new tags from above. There should be four commits: one to each of "dropshot" and "dropshot_endpoint" that sets the version to the one you specified, and one that sets the version to the _next_ version. For example, if the current version is 0.5.1 and you're publishing 0.5.2, the repo will start at 0.5.2-pre, you'll see one commit that sets it to 0.5.2, and then you'll see one that sets it to 0.5.3-pre.
+
Then undo the above step:
+
[source,text]
----
$ git reset --hard THE_COMMIT_YOU_STARTED_WITH
$ git tag -d THE_NEW_TAG
----
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these two steps cancel each other out, why bother doing them? Doesn't the dry run provide enough information about what will happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I were going to drop either the --dry-run step or the --skip-push --skip-publish step, I would skip the --dry-run because it doesn't provide any information over the other one. I believe I saw cases (while working out the config) where the dry run succeeded and the --skip-push --skip-publish failed.

I think what you're getting at (and I kind of alluded to later without saying it) is that there's a bunch of fear in this procedure. I think that fear stems from my initial experiments with cargo-release resulting in it doing things I found very surprising. I don't remember more than that, but the lesson I internalized was that I wanted to see exactly what it was doing before committing to anything. I expect others might not be so paranoid about it, which is why I started with the "if you're daring" one-liner, but kept in enough detail for somebody to do it very carefully if they wanted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, at first read I think I somehow missed that "if you want to go all-out, use this one liner" option. Makes sense to have a more intensive breakdown to see all the incremental changes, and then folks can rely on the shorthand once they (or the collective we) trust the process.

Comment on lines +108 to +109
At this point, the new crates should be published to crates.io. Check and
push the commits and the tags.
Copy link
Contributor

Choose a reason for hiding this comment

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

To make sure I'm understanding correctly: The crate is published before the commits are pushed? Is this because we're creating tags and publishing, but explicitly passing --skip-push?

Is there a reason we're avoiding letting cargo release do this for us?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is correct. The reason is what I mentioned above: I wanted to be able to check the tool's output as much as possible before committing to it. Separating this allows one to check them before pushing. (Yes, we may have already published, but that's why I also do a the non-publishing version first, too.)

The one-liner at the top of this section has cargo release do it for you.

Copy link
Contributor

@smklein smklein left a comment

Choose a reason for hiding this comment

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

Thanks for the docs / updates!

@davepacheco
Copy link
Collaborator Author

I just sync'd this up, updated Cargo.lock, and fixed the docs for the Dependabot change I made (discussed above). I plan to land this shortly.

@davepacheco davepacheco merged commit 87a27ef into main Oct 14, 2021
@davepacheco davepacheco changed the title add maintainer docs add maintainer docs and Cargo.lock Oct 14, 2021
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