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

[WIP] enforce newsfragments in CI #153

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pipermerriam
Copy link
Member

What was wrong?

How was it fixed?

To-Do

  • Clean up commit history

Cute Animal Picture

put a cute animal picture link inside the parentheses

@pipermerriam pipermerriam force-pushed the piper/ensure-towncrier-runs-in-CI branch from e4f6c8f to da7e2da Compare August 13, 2019 22:17
@pipermerriam
Copy link
Member Author

cc @cburgdorf wonder what you think about this. It catches errors like the following

This ``code` is not correctly closed

But it also errors on things like this because it doesn't know anything about the :class: directive.

    The :class:`~lahja.endpoint.Endpoint` enables communication between different processes

@@ -60,7 +60,7 @@ release: clean
# Now generate the release notes to have them included in the release commit
towncrier --yes --version $(UPCOMING_VERSION)
# Before we bump the version, make sure that the towncrier-generated docs will build
make build-docs
$(MAKE) build-docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: Why is this the preferred over just make build-docs?

Copy link

Choose a reason for hiding this comment

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

On OSes like Solaris, it was a way to use the same make command that the user typed in, which often was gmake because the Solaris make didn't understand GNU Make extensions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for filling my knowledge gap!

@cburgdorf
Copy link
Contributor

But it also errors on things like this because it doesn't know anything about the :class: directive.

Urgh..that's not good. I think we want to encourage the use of these directives to directly make pointers from release notes into relevant docs.

Actually, I thought we would already catch any relevant errors by running this in CI:

lahja/Makefile

Line 53 in 93610b2

towncrier --draft --version preview

But if towncrier itself does not validate the RST, how about we do the following:

  • instead of running towncrier --draft, write the actual changelog to disk
  • turn the order of operations so that we first generate the new changelog and then run the build-docs job

This should catch any relevant errors (we also have warnings turned into errors)

Does that make sense to you?

@pipermerriam
Copy link
Member Author

I thought about doing it that way (actual generation of the changelog) but it's a stateful and destructive action and for us to be able to do that test we'd also have to undo the changes it makes which is a bit more complexity than I hoped to add...

@cburgdorf
Copy link
Contributor

but it's a stateful and destructive action and for us to be able to do that test we'd also have to undo the changes it makes which is a bit more complexity than I hoped to add...

Why is that? It's just a thing that happens in CI but doesn't persist anywhere else (unless I'm missing something). We would generate the changelog on disk of the CI server and generate the docs (just as we do today) and either it raises errors or it doesn't. But nothing would be commited/persisted anywhere.

@pipermerriam
Copy link
Member Author

Right, but then if someone runs that code locally they end up having done a destructive action... agreed that in the CI environment it's no big deal but I'm hesitant towards an approach that can only be run in CI.

@cburgdorf
Copy link
Contributor

Right, but then if someone runs that code locally they end up having done a destructive action

If we really care about this, we can do the following:

  1. Ensure the command can not be run when the repository is in a dirty state (e.g. uncommited changes)

  2. When the check is finished, throw away the changes so that we are back at the initial state where we started off.

The main reason why I don't like the other approach is because it prevents us from writing nice release notes that make pointers into our docs.

@pipermerriam
Copy link
Member Author

The main reason why I don't like the other approach is because it prevents us from writing nice release notes that make pointers into our docs.

No arguments from me on that. Just wanted to try and see what we could do to validate proper RST formatting as in the most recent release I ran into this though it wasn't hard to fix.

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

3 participants