Issue #1633 nox session for tagging

[x1101]

First pass at creating a nox session for using the tagging tool.

Some notes

• Does not handle all the extended use cases of the tagging tool
• With not arguments will clone ansible/ansible into a tmp directory
  • Does not currently clean up this tmp directory, very open to discussions on if/how to do this
• With no arguments will run tag subcommand

This is my first pass at using nox, so if there are things I'm missing or could be improved I am very open to collaboration.

[oraNod]

Thanks for doing this @x1101 🚀 I've left some initial thoughts and will be curious to see what others have to say. Let me know what you think.

[oraNod]

I'm not sure about cloning the core repo as part of this session. The tagger script already determines the default core checkout and an extra clone feels wasteful.

This also requires you to set the --core argument when running the tagger if you want to use the existing core checkout, which seems a bit less convenient than using the tagger directly.

Maybe we could call tag.py to catch when the ansible repo doesn't exist and then do the clone?

[oraNod]

Hmm. Now I'm on the fence. I guess the clone here resolves the need to set the --remote arg if it's not origin as well as making sure that the repo is up to date.

[x1101]

Yeah, I had a lot of the same thoughts.

What I landed on for a first pass was designing around the idea of

"running nox -s tag should require little/no knowledge of how the tagging tool works under the hood, and should 'just work'"

Based on running the tag tool multiple times, I'm fairly confident I know (relative to the docs directory) where it expects core to be. I can add in a test for if it already exists either there or in the tmp location, and potentially some other checks to confirm branches etc.

[oraNod]

@x1101 that makes sense and maybe I'm coming around to the idea of doing the clone here. One thing I think it's important to avoid is making the nox session too complex and my suggestions could be pushing things down the wrong path. The nox session should only be a light wrapper to the tagger script.

[oraNod]

Does not currently clean up this tmp directory, very open to discussions on if/how to do this

If we do decide to clone core into a tmp directory, it exists inside the virtual environment that nox creates (.nox/tag/tmp) - which I suppose you already know. But we could remove that at the end of the session pretty easily with something like this before the tag session:

def _rm_tmpdir(tmpdir):
    shutil.rmtree(tmpdir, ignore_errors=True)

At the end of the tag session, you could do something like:

session.run("python", "hacking/tagger/tag.py", *args)
if tmpdir:
    _rm_tmpdir(tmpdir)

There's probably a more elegant way to do that with a try block or something. There are more experienced Pythonistas around here who are better at that stuff but at least it works. 🤷

[x1101]

Does not currently clean up this tmp directory, very open to discussions on if/how to do this

If we do decide to clone core into a tmp directory, it exists inside the virtual environment that nox creates (.nox/tag/tmp) - which I suppose you already know. But we could remove that at the end of the session pretty easily with something like this before the tag session:

def _rm_tmpdir(tmpdir):
    shutil.rmtree(tmpdir, ignore_errors=True)

At the end of the tag session, you could do something like:

session.run("python", "hacking/tagger/tag.py", *args)
if tmpdir:
    _rm_tmpdir(tmpdir)

There's probably a more elegant way to do that with a try block or something. There are more experienced Pythonistas around here who are better at that stuff but at least it works. 🤷

That's already better than what I initially had (and removed).

I've got a few ideas that I'll work into this once I have a chance.

[oraNod]

Thanks for giving it another go @x1101 I think we're making progress. Hopefully you don't mind me going back and forth a little in the review. Sometimes it's hard to settle on an approach until you play around.

[oraNod]

Suggested change

	Determine the missing ansible-core releases,
	create corresponding tags for each release in the ansible-documentation repo,
	and then push them
	Check the core repo for new releases and create tags in ansible-documentation.

[oraNod]

I'd shorten this a bit and keep it all on a single line so all the text in the description is visible with nox --list.

[oraNod]

We should probably have some try blocks in here to handle errors a little more gracefully. Something like:

try:
  session.run("git", "-C", CHECK_LOCATION, "pull", "--quiet", external=True)
  args.extend(["--core", str(CHECK_LOCATION)])
except Exception as e:
  session.error(f"Could not update the core repository: {e}")

However, in my case, I ran into fatal: Need to specify how to reconcile divergent branches. when trying this out. I think to overcome that we'd need to use a pull strategy in the nox session. I'm a --rebase kind of guy but other folks have different preferences so it's tricky to find any one right way to handle this.

In any event I guess the larger concern here might be that a pull could risk wiping local changes or creating a mess on someone's branch. It seems we're also going down the road of adding extending the tagger via the nox session, which is something I'd prefer not to do. Although I know it's sort of my fault for suggesting we compare the local checkout with the upstream core repo in an earlier comment.

Git operations can be finicky so maybe it's best to go with your initial approach of cloning into the virtual environment. If we can remove the clone when we're done that probably takes care of the concern about being a bit wasteful.

[oraNod]

Suggested change

	session.run("python", "hacking/tagger/tag.py", *args)
	if tmpdir:
	_rm_tmpdir(tmpdir)
	try:
	session.run("python", "hacking/tagger/tag.py", *args)
	finally:
	if tmpdir:
	_rm_tmpdir(tmpdir)

[oraNod]

This should remove the tmp directory even if the session fails.

[oraNod]

Added a few others for review. @felixfontein as you have created tags before I think your take on this would be useful. Cheers.

[x1101]

Thanks for giving it another go @x1101 I think we're making progress. Hopefully you don't mind me going back and forth a little in the review. Sometimes it's hard to settle on an approach until you play around.

Absolutely agree.

Looking over your list of comments, I think we can solve several at once by

• deciding that we're just going to clone into a nox-specific venv
• clone both ansible and ansible-docs
• clean them up when we're done

Then add the bits about

• checking if git is installed
• initializing tmpdir to None
• wrapping things in try/catch
• removing both temp repos when done
• shorten the session summary to one line

Thanks for your patience with missing some obvious python best practices. I'm not a developer by trade, and I'm still building up those skills.

[x1101]

So just trying to summarize to see if I understand this:

1. clone core (to get the latest tags etc)

2. run this nox session. If it gives output, then there were tags in core, if not, there were no new tags

3. Can always run script directly with --newtags option to see first if there are tags

Is that about right?

Based on what @gotmax23 has said, the ideal session is

• user already has docs and core cloned
• runs nox -s tag
• tagging happens

the --newtags option to the tagger script sounds to me like its a troubleshooting/debugging, rather than part of the expected workflow (which is why --newtag isn't part of the current doc process either).

If you run nox -s tag and there are no new tags, it doesn't fail, it does 0 of the 0 things to do. (and exits successfully)

[oraNod]

I've been in the weeds with other stuff but was thinking about this PR earlier today when I noticed a ping from someone in the docs channel about missing tags in the repo. It occurred to me that we could perhaps use this session in our ci to periodically check for new tags in ansible/ansible and notify on Matrix. Maybe other folks would have a different view but, from the vantage of usefulness, giving the DaWGs a heads up about missing tags seems like it addresses a need. Just putting it out there.

[x1101]

I've been in the weeds with other stuff but was thinking about this PR earlier today when I noticed a ping from someone in the docs channel about missing tags in the repo. It occurred to me that we could perhaps use this session in our ci to periodically check for new tags in ansible/ansible and notify on Matrix. Maybe other folks would have a different view but, from the vantage of usefulness, giving the DaWGs a heads up about missing tags seems like it addresses a need. Just putting it out there.

My first thought is "that's a different enough use case that it should be a different nox session. One I think we should have, but not this one".

[samccann]

So I think clone core is required, otherwise the user can be trying this w/o remembering to clone core (and thus have no new tags). Though I suppose I'd figure it out soon enuf if someone (or some bot) said 'missing tags' and I ran this nox session and nothing changed :-)

As for something to remind us in matrix, yes please! I tend to have an eye out for core releases anyway, but not RCs, betas, etc so a nagogram reminder would be most welcome (whereever it runs..not saying part of this PR).

[gotmax23]

The process for running the nox session (and the prerequisite of cloning the repositories) is now documented in the README.

[gotmax23]

@samccann, let me know if those docs answers your questions. See also #1649 (comment).

@x1101, I pushed a change to document this in the README. Thank you for working on this.

Thanks for waiting for me to review this after I get back from vacation.

[oraNod]

Thanks a mill for the work on this one @x1101 🚀 I'm still interested in seeing if we can use this in ci to periodically check for missing tags and notify the dawgs but it's definitely a follow on PR. I do think this session could be useful for that and provides a bit of inspiration if nothing else. Cheers!

[samccann]

thanks @x1101 and @gotmax23. It's all making sense to me now!

[webknjaz]

Instead of making a list and mutating it, why not make an additional var just for those extra args?

Suggested change

	args = list(session.posargs)
	# If run without any arguments, default to "tag"
	if not any(arg.startswith(("hash", "mantag", "new-tags", "tag")) for arg in args):
	args.append("tag")
	session.run("python", "hacking/tagger/tag.py", *args)
	known_cli_args_present = any(arg.startswith(("hash", "mantag", "new-tags", "tag")) for arg in session.posargs)
	extra_cli_args = () if known_cli_args_present else ("tag",)
	session.run("python", "hacking/tagger/tag.py", *session.posargs, *extra_cli_args)

[x1101]

I was matching the existing pip-compile session that does the same args.append(). I'm personally in favor of keeping things consistent. If we want to change it, we should change both, possibly as a separate effort from this.

[oraNod]

@webknjaz Thanks for taking a look and providing some nits. I agree with @x1101 that your other suggestion would make a good follow up so we have consistency. Any final comments on your part? I'd rather we go ahead and merge this as soon as we can. Cheers.

[oraNod]

My bad, @gotmax23 I accidentally re-requested a review from you.

[gotmax23]

Thanks!

[oraNod]

Thanks @x1101 and @gotmax23 !