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

install: record version control information #3849

Closed
oliver-sanders opened this issue Oct 1, 2020 · 13 comments · Fixed by #4142
Closed

install: record version control information #3849

oliver-sanders opened this issue Oct 1, 2020 · 13 comments · Fixed by #4142
Assignees
Milestone

Comments

@oliver-sanders
Copy link
Member

oliver-sanders commented Oct 1, 2020

closely related to #3819

Re-implement the Rose ability to record version control information.

Write a plugin (using the on-install entry point developed in #3819) that runs on workflow installation to detect if the workflow is being installed from a version controlled working copy and if so record the revision along with any un-committed diff.

Note: The plugin will probably look like this:
install(src_dir: Path, run_dir: Path, run_mode: str) -> None

This info should get dumped into a file somewhere in the workflow run directory.

This should work for git and svn.

Questions:

  • Where should we dump this info?
    1. ~/<flow>/log/version-control/<timestamp>-<run_mode>
    2. Somewhere else?
  • Are there any nice abstractions which would save us having to support each VC system individually?
@oliver-sanders oliver-sanders added the question Flag this as a question for the next Cylc project meeting. label Oct 1, 2020
@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Oct 1, 2020
@hjoliver
Copy link
Member

hjoliver commented Oct 6, 2020

Questions:

  • Where should we dump this info?
    i. ~//log/version-control/-<run_mode>
    ii. Somewhere else?

Seems sensible. Maybe remove the -control suffix and store Cylc version there too?

  • Are there any nice abstractions which would save us having to support each VC system individually?

It seems there's stuff out there, but it might be overkill for our simple requirements. (Unless we want to do more than just interrogate the version). https://github.com/vcs-python/libvcs

@oliver-sanders
Copy link
Member Author

Happy with overkill so long as it does the job. I found a couple of others in my travels, though finding one which is able to obtain the staged/unstaged diff may by difficult.

Ideally we would support at least git, svn and hg.

@MetRonnie MetRonnie self-assigned this Nov 24, 2020
@MetRonnie
Copy link
Member

Am I right in my understanding that this will need a new event type install like these existing ones?

Event Types
^^^^^^^^^^^
Coroutines must be decorated using one of the main loop decorators. The
choise of decorator effects when the coroutine is called and what
arguments are provided to it.
The available event types are:
.. autofunction:: cylc.flow.main_loop.startup
.. autofunction:: cylc.flow.main_loop.shutdown
.. autofunction:: cylc.flow.main_loop.periodic
"""

And if so, is @wxtim working on that as part of #3819? Or should I have a go?

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Nov 25, 2020

Yes, this would use the post-install entry point that is being created in #3819 where the interface would look something like:

post_install(src_dir, run_dir, cli_opts) -> dict

@MetRonnie
Copy link
Member

MetRonnie commented Nov 25, 2020

If we can't find a library to do this for us, things we need to be careful of:

  • git worktrees
  • git submodules

@MetRonnie MetRonnie removed the question Flag this as a question for the next Cylc project meeting. label Dec 10, 2020
@oliver-sanders
Copy link
Member Author

Looks like there isn't a nice library we can grab for purpose, will need to implement from scratch, shouldn't be too bad (famous last words), the required SVN functionality was provided by Rose2019.

Information we need to capture:

  • hash/revision
  • Any uncommitted diff against that hash/revision
  • The URL for SVN repos.
  • (untracked files?)

Would suggest dumping the diff (if present) in a patch file and the metadata in a Cylc/Rose readable (non-nested) INI-like format.

@MetRonnie
Copy link
Member

Do we want to include the diff of untracked files in the case of git? (It's doable but there isn't a straightforward command for getting the diff of untracked files, surprisingly)

@oliver-sanders
Copy link
Member Author

I was just thinking that from a provenance perspective perspective it would be good to have a note somewhere recording that there were untracked files as these could very well affect results. Just listing them would be enough if we were to do this, purposefully ignored files (e.g. .gitignore should be omitted).

I don't think Rose2019 records this information so not needed for Cylc8 but a nice-to-have anyway.

@MetRonnie
Copy link
Member

MetRonnie commented Jan 5, 2021

Would suggest dumping the diff (if present) in a patch file and the metadata in a Cylc/Rose readable (non-nested) INI-like format.

How should this be for multiline values, like statuses?

status = """
M       app/ancil_sstice/rose-app.conf
M       app/um/opt/rose-app-stashpack1.conf
M       app/um/rose-app.conf
M       rose-suite.conf
"""

or

status=M       app/ancil_sstice/rose-app.conf
      =M       app/um/opt/rose-app-stashpack1.conf
      =M       app/um/rose-app.conf
      =M        rose-suite.conf

Personally, I prefer the former

@MetRonnie
Copy link
Member

Is this plugin supposed to be enabled by default? (With what I've written so far, it is enabled by default, and I'm not sure how it would be made opt-in)

@oliver-sanders
Copy link
Member Author

I don't think there is a particularly good use case for turning this plugin off.

Off topic comment on plugins: We don't have a fancy system for enabling/disabling plugins yet. I think we will hit the limits of entry points pretty quickly and should think about using something like pluggy and building in some fancy way to configure what plugins are used but that's fun for another day.

@hjoliver
Copy link
Member

hjoliver commented Mar 1, 2021

@oliver-sanders is that quoted "Off topic comment on plugins" coming from you? (It seems somewhat at odds with the un-quoted statement that precedes it!)

@oliver-sanders
Copy link
Member Author

The operating words there were this plugin, trying to avoid polluting this issue with a larger plugin debate so I've dumped some ideas I've been batting around for a while into an issue - #4106.

In this case I don't think the plugin really requires any configuration, however, other plugins might and the main loop plugins already do. I had a look into centralising the plugin functionality a while back but couldn't come up with an ideal solution (the more you configure up front the more work Cylc needs to do up front).

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 a pull request may close this issue.

3 participants