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

Restrict installation logging to workflow srcdir #4423

Merged

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Sep 21, 2021

These changes close #4220
This is a small change with no associated Issue.

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).
  • Applied any dependency changes to both setup.py and
    conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation PR Req'd: This feature is documented in the plugin module docstring which is ingested by cylc-doc. I have not changed this, because this change makes the description less misleading.

@wxtim wxtim force-pushed the restrict-installation-logging-to-workflow-srcdir branch from 0d20c13 to c772965 Compare September 21, 2021 16:34
@wxtim wxtim force-pushed the restrict-installation-logging-to-workflow-srcdir branch from c772965 to 56ee05d Compare September 21, 2021 16:35
@wxtim wxtim self-assigned this Sep 21, 2021
@wxtim wxtim marked this pull request as draft September 21, 2021 16:37
@wxtim wxtim marked this pull request as ready for review September 21, 2021 19:59
@wxtim wxtim requested review from hjoliver and kinow September 21, 2021 20:07
@wxtim wxtim added the small label Sep 21, 2021
@wxtim wxtim modified the milestones: cylc-8.0b3, cylc-8.0rc1 Sep 21, 2021
@hjoliver
Copy link
Member

@wxtim your new tests pass locally for me, but @oliver-sanders's example on #4220 fails with this diff in myproject:

┬─[oliverh@niwa-1007885:~/c/myproject]─[12:31:09]─[I]─[G:master]
╰─>$ pwd
/home/oliverh/cylc-src/myproject
┬─[oliverh@niwa-1007885:~/c/myproject]─[12:31:11]─[I]─[G:master]
╰─>$ git diff
diff --git a/build/workflow/flow.cylc b/build/workflow/flow.cylc
index e69de29..907b308 100644
--- a/build/workflow/flow.cylc
+++ b/build/workflow/flow.cylc
@@ -0,0 +1 @@
+blah
diff --git a/src/a b/src/a
index e69de29..907b308 100644
--- a/src/a
+++ b/src/a
@@ -0,0 +1 @@
+blah
diff --git a/test-workflow/flow.cylc b/test-workflow/flow.cylc
index e69de29..907b308 100644
--- a/test-workflow/flow.cylc
+++ b/test-workflow/flow.cylc
@@ -0,0 +1 @@
+blah

Result:

┬─[oliverh@niwa-1007885:~/cylc-src]─[12:33:06]─[N]
╰─>$ cylc install -C myproject/test-workflow
INSTALLED test-workflow/run1 from myproject/test-workflow
PluginError: Error in plugin cylc.post_install.log_vc_info
[Errno 128] fatal: ambiguous argument 'myproject/test-workflow': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

It works on master, albeit with the out-of-src-dir diffs logged.

@hjoliver
Copy link
Member

Looks like your change assumes an absolute source path on the cylc install command line.

@hjoliver hjoliver modified the milestones: cylc-8.0rc1, cylc-8.0b3 Sep 22, 2021
…tion-logging-to-workflow-srcdir

* upstream/master: (96 commits)
  Update cylc/flow/cfgspec/workflow.py
  clean up interim obselete message
  fix flake8-simplify failures caused by recently added checks. (cylc#4420)
  Fix Change log
  Add test and raise error on src dir move
  change log
  Source symlink fix
  publish pending flag
  handle orphan tasks post reload
  Create window elements & load jobs from DB
  Deltas batched post reload
  Fix a doctest.
  Add colour to GH Actions pytest log
  Address review feedback.
  Type annotations for timer.
  Correct func test timeouts.
  Fix test.
  Add Cylc 7 back-compat func tests.
  Add stall restart func test.
  Func test for workflow timeout.
  ...
@wxtim
Copy link
Member Author

wxtim commented Sep 22, 2021

Looks like your change assumes an absolute source path on the cylc install command line.

I think that I have fixed this.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Using master, after re-creating the scenario described in the issue linked (also git add and commit, then modify a file), uncommitted.diff:

# Auto-generated diff of uncommitted changes in the Cylc workflow repository:
#   myproject/test-workflow
diff --git a/build/workflow/flow.cylc b/build/workflow/flow.cylc
index 56f0067..0275ba1 100644
--- a/build/workflow/flow.cylc
+++ b/build/workflow/flow.cylc
@@ -1,3 +1,7 @@
+
+
+
+
 [scheduling]
   cycling mode = integer
   initial cycle point = 1

Using this branch, uncommitted.diff:

# Auto-generated diff of uncommitted changes in the Cylc workflow repository:
#   myproject/test-workflow

So not displaying the changes in the other directory 🎉 ! vcs.conf still shows the changes, but I think that's OK:

version control system = "git"
repository version = "81f03d9-dirty"
commit = "81f03d9c7c83399a44216e8db3272b7475a46afa"
working copy root path = "myproject/test-workflow"
status = """
 M ../build/workflow/flow.cylc
"""

+1

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Noice 💯

@hjoliver hjoliver merged commit 7e47331 into cylc:master Sep 24, 2021
@wxtim wxtim deleted the restrict-installation-logging-to-workflow-srcdir branch March 22, 2022 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

log version control: diff from outside the workflow source dir listed
3 participants