Skip to content

Add columns for common arguments, descriptions #563

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add columns for common arguments, descriptions #563

wants to merge 2 commits into from

Conversation

voetberg
Copy link
Contributor

Partially closes #499

Rest would need to be addressed with changes to core.

rdimaio
rdimaio reviewed May 21, 2025
View reviewed changes
rdimaio
rdimaio reviewed May 21, 2025
View reviewed changes
@voetberg @rdimaio
Apply suggestions from code review
7a392c5 
Co-authored-by: Riccardo Di Maio <[email protected]>
rdimaio
rdimaio approved these changes May 22, 2025
View reviewed changes
Copy link
Contributor

@rdimaio rdimaio left a comment

Choose a reason for hiding this comment

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

LGTM, not sure what's going on with the CI

@Geogouz
Copy link
Contributor

Geogouz commented May 22, 2025 •
edited
Loading

LGTM, not sure what's going on with the CI

YAML parsing problems.
We could use maybe multi-line YAML strings to avoid parser confusion?

namely for example here, go from:

description: defines how to handle datasets: `fifo` (each file released separately) or `grouped_fifo` (wait for the entire dataset to fit)

to something like:

description: |
  defines how to handle datasets: `fifo` (each file released separately)
  or `grouped_fifo` (wait for the entire dataset to fit)

@Geogouz Geogouz mentioned this pull request May 22, 2025
Merged
@rdimaio
Copy link
Contributor

rdimaio commented May 22, 2025

LGTM, not sure what's going on with the CI

YAML parsing problems. We could use maybe multi-line YAML strings to avoid parser confusion?

namely for example here, go from:

description: defines how to handle datasets: `fifo` (each file released separately) or `grouped_fifo` (wait for the entire dataset to fit)

to something like:

description: |
  defines how to handle datasets: `fifo` (each file released separately)
  or `grouped_fifo` (wait for the entire dataset to fit)

Yeah, I think ideally it would be best to fix this on the doc generation side, rather than having to think about this whenever we write docstrings in the main code

@voetberg
Copy link
Contributor Author

LGTM, not sure what's going on with the CI

YAML parsing problems. We could use maybe multi-line YAML strings to avoid parser confusion?
namely for example here, go from:

description: defines how to handle datasets: `fifo` (each file released separately) or `grouped_fifo` (wait for the entire dataset to fit)

to something like:

description: |
  defines how to handle datasets: `fifo` (each file released separately)
  or `grouped_fifo` (wait for the entire dataset to fit)

Yeah, I think ideally it would be best to fix this on the doc generation side, rather than having to think about this whenever we write docstrings in the main code

Ideally this should be caught by tools/check_rest_api_documentation.sh in the main repo - is there a reason that isn't run as part of the testing suite? It's use is described in our developer docs

@rdimaio
Copy link
Contributor

rdimaio commented May 23, 2025

LGTM, not sure what's going on with the CI

YAML parsing problems. We could use maybe multi-line YAML strings to avoid parser confusion?
namely for example here, go from:

description: defines how to handle datasets: `fifo` (each file released separately) or `grouped_fifo` (wait for the entire dataset to fit)

to something like:

description: |
  defines how to handle datasets: `fifo` (each file released separately)
  or `grouped_fifo` (wait for the entire dataset to fit)

Yeah, I think ideally it would be best to fix this on the doc generation side, rather than having to think about this whenever we write docstrings in the main code

Ideally this should be caught by tools/check_rest_api_documentation.sh in the main repo - is there a reason that isn't run as part of the testing suite? It's use is described in our developer docs

We could add that to the main repo's CI, but is there no way to fix this on the generation side, e.g. ignoring every colon after the first?

@Geogouz Geogouz mentioned this pull request May 23, 2025
Open
@Geogouz
Copy link
Contributor

Geogouz commented May 23, 2025

LGTM, not sure what's going on with the CI

YAML parsing problems. We could use maybe multi-line YAML strings to avoid parser confusion?
namely for example here, go from:

description: defines how to handle datasets: `fifo` (each file released separately) or `grouped_fifo` (wait for the entire dataset to fit)

to something like:

description: |
  defines how to handle datasets: `fifo` (each file released separately)
  or `grouped_fifo` (wait for the entire dataset to fit)

Yeah, I think ideally it would be best to fix this on the doc generation side, rather than having to think about this whenever we write docstrings in the main code

Ideally this should be caught by tools/check_rest_api_documentation.sh in the main repo - is there a reason that isn't run as part of the testing suite? It's use is described in our developer docs

We could add that to the main repo's CI, but is there no way to fix this on the generation side, e.g. ignoring every colon after the first?

We can maybe continue about it here #567

mgajek-cern
mgajek-cern reviewed Sep 12, 2025
View reviewed changes
docs/started/main_components/daemons.md
rucio-necromancer | Deletion | Works on permanently unavailable replicas, it tries to recover the data from other valid replicas if any, else declares the replica as lost | ✅ | ✅ | ❌ | [Details](bin/rucio-necromancer.md)
rucio-oauth-manager | Auth/Authz | Deletes expired access tokens (in case there is a valid refresh token, expired access tokens will be kept until refresh_token expires as well.) and deletion of expired OAuth session parameters | ✅ | ✅ | ❌ | [Details](bin/rucio-oauth-manager.md)
rucio-reaper | Deletion | Deletes replicas that don't have locks anymore, i.e. they have a tombstone set | ✅ | ✅ | ✅ | [Details](bin/rucio-reaper.md)
rucio-suspicious-replica-recoverer | Replica | Declares suspicious replicas as bad if they are found available on other RSEs, so necromancer will work on them | ❌ | ❌ | ✅ | [Details](bin/rucio-replica-recoverer.md)
Copy link

Choose a reason for hiding this comment

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

nit: I notice the daemon name rucio-suspicious-replica-recoverer in the table doesn't match the actual executable rucio-replica-recoverer that exists in the codebase. Should we update the table to use the correct executable name, or is there a reason for this discrepancy?

Copy link
Member

Choose a reason for hiding this comment

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

Should be updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bari12 bari12 bari12 left review comments

@mgajek-cern mgajek-cern mgajek-cern left review comments

@rdimaio rdimaio rdimaio 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.

Add docs for daemon arguments
5 participants
@voetberg @Geogouz @rdimaio @bari12 @mgajek-cern