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.

Sign up for GitHub

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

Jump to bottom

tickets/DM-43579: Move runlab.sh functionality into lsst.rsp.startup #36

Merged
merged 4 commits into from
Apr 11, 2024
Merged

tickets/DM-43579: Move runlab.sh functionality into lsst.rsp.startup #36

merged 4 commits into from
Apr 11, 2024

Conversation

athornton
Copy link
Member

@athornton athornton commented Apr 5, 2024 •
edited
Loading

This has a companion PR in sciplat-lab (lsst-sqre/sciplat-lab#169) to remove the stuff we moved into lsst.rsp and launch from this one's "launch-rubin-jupyterlab" entrypoint.

@athornton athornton requested a review from rra April 5, 2024 23:22
@athornton athornton mentioned this pull request Apr 8, 2024
Merged
@athornton
Copy link
Member Author

cf. lsst-sqre/sciplat-lab#169

@athornton
Copy link
Member Author

It also launches labs successfully now.

rra
rra reviewed Apr 9, 2024
View reviewed changes
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

I would not include a git repository, even a bare one, inside another git repository. It's going to be a nightmare of tools getting confused. I'm somewhat surprised GitHub even allows that and doesn't prevent you from doing it entirely.

Makefile Outdated Show resolved Hide resolved
src/lsst/rsp/startup/__init__.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/lsst/rsp/startup/cli.py Outdated Show resolved Hide resolved
src/lsst/rsp/startup/constants.py Outdated Show resolved Hide resolved
src/lsst/rsp/startup/storage/process.py Outdated Show resolved Hide resolved
src/lsst/rsp/startup/storage/process.py Outdated Show resolved Hide resolved
src/lsst/rsp/startup/util.py Outdated Show resolved Hide resolved
src/lsst/rsp/utils.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
@athornton
Copy link
Member Author

athornton commented Apr 9, 2024 •
edited
Loading

I would not include a git repository, even a bare one, inside another git repository. It's going to be a nightmare of tools getting confused. I'm somewhat surprised GitHub even allows that and doesn't prevent you from doing it entirely.

So a better approach would be a fixture that initialized a new repository in a tempdir and then yielded it? That's what I ended up doing.

@athornton athornton force-pushed the tickets/DM-43579 branch 5 times, most recently from b4bec02 to a7803f1 Compare April 10, 2024 22:41
@athornton athornton requested a review from rra April 10, 2024 23:26
rra
rra approved these changes Apr 11, 2024
View reviewed changes
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

Just a few minor things left, but overall this looks great. Thank you so much for doing this!

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/lsst/rsp/startup/services/labrunner.py Outdated Show resolved Hide resolved
@athornton athornton merged commit 07786e8 into main Apr 11, 2024
5 checks passed
@athornton athornton deleted the tickets/DM-43579 branch April 11, 2024 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rra rra rra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@athornton @rra