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

Record workflow version control information post-install #4142

Merged
merged 13 commits into from
Apr 28, 2021

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Mar 24, 2021

These changes close #3849

On installation of a workflow, will record the version control status to log/version/vcs.conf and any uncommitted diff to log/version/uncommitted.diff.

Supports git and svn.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included
  • (master branch) I have opened a documentation PR at Autodocument post-install plugins cylc-doc#236
  • No dependency changes.

@MetRonnie MetRonnie added this to the cylc-8.0b1 milestone Mar 24, 2021
@MetRonnie MetRonnie self-assigned this Mar 24, 2021
@MetRonnie

This comment has been minimized.

@MetRonnie MetRonnie force-pushed the record-vc branch 4 times, most recently from 670f005 to 4488a2e Compare March 24, 2021 18:37
@MetRonnie MetRonnie marked this pull request as draft March 24, 2021 18:37
@MetRonnie MetRonnie force-pushed the record-vc branch 4 times, most recently from cbd0350 to 7979ff8 Compare March 25, 2021 13:12
"""
if not info:
raise ValueError("Nothing to write")
info_file = Path(run_dir, 'log', 'version', 'vcs.conf')
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see a way to get the timestamp of the installation, perhaps this needs to be added to the entry point arguments?

@MetRonnie MetRonnie marked this pull request as ready for review March 25, 2021 13:15
@MetRonnie MetRonnie marked this pull request as draft March 26, 2021 16:58
@MetRonnie

This comment has been minimized.

@MetRonnie MetRonnie marked this pull request as ready for review March 29, 2021 08:25
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

Minor comments. I have tested the change as working, read the code and run the tests. Thanks Ronnie.

return None


def _run_cmd(vcs: str, args: Iterable[str], cwd: Union[Path, str]) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth making use of run_cmd in remote.py? Could adapt it if you need to pass the cwd?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, but it wouldn't quite work here due to the difference in error handling

cylc/flow/post_install/log_vc_info.py Outdated Show resolved Hide resolved
@MetRonnie
Copy link
Member Author

Just remembered a question I had: do we want to save the diff of untracked files for a git source dir? This is actually not the default behaviour of git, git diff HEAD only gets the diff of tracked files (unstaged + staged)

@datamel
Copy link
Contributor

datamel commented Mar 30, 2021

Just remembered a question I had: do we want to save the diff of untracked files for a git source dir? This is actually not the default behaviour of git, git diff HEAD only gets the diff of tracked files (unstaged + staged)

Could those untracked files make a difference to the running of the workflow? For example, if they had configured their .cylcignore file to ignore directories for the installation but this was not under source control, they may wish to know about it. Although perhaps this could lead to the log filling with unwanted diffs... difficult choice. I guess by marking files as untracked they should assume it would not be included in the diff, perhaps a one line warning at the top reminding them that there may be untracked changed?

@MetRonnie
Copy link
Member Author

MetRonnie commented Mar 30, 2021

Could those untracked files make a difference to the running of the workflow?

Yes

I guess by marking files as untracked they should assume it would not be included in the diff, perhaps a one line warning at the top reminding them that there may be untracked changed?

I don't think there is a way of marking files as untracked. It's pretty much only newly-created files that haven't been git added yet that are untracked.

The untracked files will still show up in the vcs.conf file with ?? status, e.g.

status = """
?? foo/bar.txt
"""

@hjoliver hjoliver modified the milestones: cylc-8.0b1, cylc-8.0b2 Apr 15, 2021
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looking good, just need to give it a spin.

Built In Plugins
----------------

Cylc Flow provides the following post-install plugins:
Copy link
Member

Choose a reason for hiding this comment

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

We can document the pre-install and post-install entry points together as "install plugins" since they have related functionality and plugins may use both (e.g. cylc-rose).

tests/unit/post_install/test_log_vc_info.py Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Tested with Git, all good.

  • Needs a quick changelog entry.
  • Can you squash down commits, namely "Tidy" ones.


"""Record version control information on workflow install.

The supported version control systems are git and svn.
Copy link
Member

Choose a reason for hiding this comment

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

From cylc/cylc-doc#236:

Could do with a little more documentation here, a quick outline of:

  • Where the version control information is stored.
  • The format of this file.
  • The meaning of each field.

Can be quite short.

Copy link
Member Author

Choose a reason for hiding this comment

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

repository version = "2.8.0-dirty"
commit = "e5dc6573dd70cabd8f973d1535c17c29c026d553"
working copy root path = "~/cylc-src/my-workflow-git"
status = \"\"\"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these quotes should be escaped here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the whole docstring is a triple quote string? Rendering fine:

@oliver-sanders oliver-sanders merged commit 141e944 into cylc:master Apr 28, 2021
@MetRonnie MetRonnie deleted the record-vc branch April 28, 2021 15:33
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.

install: record version control information
4 participants