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

Re-enable Rosie ID. #2510

Merged
merged 6 commits into from
Dec 7, 2021
Merged

Re-enable Rosie ID. #2510

merged 6 commits into from
Dec 7, 2021

Conversation

wxtim
Copy link
Contributor

@wxtim wxtim commented Nov 30, 2021

Closes #2432

Extract version control information from installed Cylc8 workflows for the rosie id command.

Depends on: cylc/cylc-flow#3849

Requires #2509 merger to pass

  • Re-written basic functionality.
  • Re-enables t/rosie-id/00-basic.t tests 31-33 which tests functionality.

@wxtim wxtim self-assigned this Nov 30, 2021
@wxtim wxtim marked this pull request as draft November 30, 2021 09:47
@wxtim wxtim requested review from datamel and MetRonnie November 30, 2021 16:13
@wxtim wxtim marked this pull request as ready for review November 30, 2021 16:13
metomi/rosie/suite_id.py Outdated Show resolved Hide resolved
metomi/rosie/suite_id.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from datamel December 1, 2021 10:53
metomi/rosie/suite_id.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from datamel December 1, 2021 11:19
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.

The tests are failing for me locally with: location not associated with a suite ID
Are they passing for you @wxtim?

@datamel
Copy link
Contributor

datamel commented Dec 6, 2021

The tests are failing for me locally with: location not associated with a suite ID

Update: Tried again this morning with a fresh environment and this has fixed my problems @wxtim. I think pip installing (including cache clearing) everything is not enough sometimes.

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.

Thanks @wxtim. Working as expected now for me.

t/rosie-id/00-basic.t Outdated Show resolved Hide resolved
t/rosie-id/00-basic.t Outdated Show resolved Hide resolved
@oliver-sanders oliver-sanders added this to the 2.0rc1 milestone Dec 6, 2021
Copy link
Contributor

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

1 question - not a blocker

vcsystem = None
url = None
rev = None
for line in fpath.read_text().split('\n'):
Copy link
Contributor

@MetRonnie MetRonnie Dec 6, 2021

Choose a reason for hiding this comment

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

Is it not possible to parse this using Rose's parser? I guess not because Rose doesn't allow multiline strings like

status = """
 M flow.cylc
 M whatever.txt
"""

?

Then perhaps it would make sense to save VCS info as JSON instead, as you suggested. Which I think would have to be a Cylc 8.0rc1 PR...

Copy link
Contributor Author

@wxtim wxtim Dec 7, 2021

Choose a reason for hiding this comment

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

It's an internal which users shouldn't see: so IMO we should create an issue to change this beyond rc1. I don't think that it needs to be a blocker.
If we want to be failsafe I can't think that it'd be massively expensive to write this + json both.

cylc/cylc-flow#4546

@wxtim wxtim merged commit 637d8c5 into metomi:master Dec 7, 2021
@wxtim wxtim deleted the rosie_id branch December 7, 2021 09:13
@wxtim wxtim mentioned this pull request Dec 16, 2021
6 tasks
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.

rosie: Cylc8 support for rosie id
4 participants