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

Initial code checkin #2

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Initial code checkin #2

wants to merge 18 commits into from

Conversation

srp3rd
Copy link
Contributor

@srp3rd srp3rd commented Dec 8, 2023

No description provided.

Copy link

@ktlim ktlim left a comment

Choose a reason for hiding this comment

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

Stumbled back across this after a long time. I still haven't gotten to the code itself, but I have some comments on the build/deploy portions.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
repo: /tmp/repo

rses:
XRD1:
Copy link

Choose a reason for hiding this comment

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

It can sometimes be a bit awkward to have YAML keys that are user-provided. It makes it difficult to validate the files, and the syntax can sometimes be tricky if there are special characters in the key name.

The alternate way of doing this would be to have rses be a list rather than a mapping, with each list element being a mapping that contains an element like name: XRD1 in addition to rucio_prefix: and fs_prefix:.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back on forth on this when I originally did it. I didn't want to list the RSE names in two places because it was just another place to make a mistake. Having said that, I can change it if you prefer.

bin.src/ingestd Outdated Show resolved Hide resolved
.github/workflows/build.yaml Outdated Show resolved Hide resolved
docker/ingestd/Dockerfile Outdated Show resolved Hide resolved
WORKDIR /home/lsst

ARG CTRL_INGESTD_GIT_VERSION
RUN git clone https://github.com/lsst-dm/ctrl_ingestd && \
Copy link

Choose a reason for hiding this comment

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

Better to eups distrib install this if possible. Then the whole scons command can go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where are the instructions to get this to work via eups distrib install?

@@ -0,0 +1,4 @@
#!/bin/bash
source /opt/lsst/software/stack/loadLSST.bash
setup -r /home/lsst/ctrl_ingestd
Copy link

Choose a reason for hiding this comment

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

If this is installed with eups distrib install instead of git clone, then I think this can be just setup ctrl_ingestd. (Possibly you'll need to make sure the current tag is applied in eups.)

docker/testenv-ingestd/Dockerfile Outdated Show resolved Hide resolved
Change to use mamba instead of pip to install
Remove nc (no longer used)
Update version
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.

None yet

2 participants