Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add vo-list command #203

Merged
merged 5 commits into from
Jul 4, 2024
Merged

Add vo-list command #203

merged 5 commits into from
Jul 4, 2024

Conversation

sebastian-luna-valero
Copy link
Collaborator

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Added a new command to filter sites in the EGI Federated Cloud supporting a VO

Copy link
Owner

@tdviet tdviet left a comment

Choose a reason for hiding this comment

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

@sebastian-luna-valero thank very much for the PR. My first thought of fedcloud site vo-list is about listing the VO supported on a given site. What about merging the new command which the fedcloud site list command, i.e. fedcloud site list --vo VO where if the --vo option is omitted, it will print all sites, if present, print only sites supported the VO.

PS. Sorry for unfinished comments, misclicking :-)

@sebastian-luna-valero
Copy link
Collaborator Author

sebastian-luna-valero commented Jul 3, 2024

Thanks for your feedback @tdviet

What about merging the new command which the fedcloud site list command, i.e. fedcloud site list --vo VO where if the --vo option is omitted, it will print all sites, if present, print only sites supported the VO.

That was my initial attempt, but I then realised that the vo_params decorator is required:

Therefore, adding it to the existing list command for sites will return:

$ fedcloud site list
Usage: fedcloud site list [OPTIONS]
Try 'fedcloud site list --help' for help.

Error: Missing option '--vo'.

That's why I created a new command. However, I agree the naming might be confusing. Instead, how should I call it to make it more intuitive?

@tdviet
Copy link
Owner

tdviet commented Jul 3, 2024

What about creating a new decorator for --vo parameter without the required, e.g. vo_params_unrequired. I don't think there will be any conflicts between decorators

@sebastian-luna-valero
Copy link
Collaborator Author

sebastian-luna-valero commented Jul 3, 2024

Done!

I don't think there will be any conflicts between decorators

That was exactly my concern and I didn't test it. Seems to work well, please check.

Copy link
Owner

@tdviet tdviet left a comment

Choose a reason for hiding this comment

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

LGTM

@tdviet tdviet merged commit af70cec into master Jul 4, 2024
18 checks passed
@sebastian-luna-valero sebastian-luna-valero deleted the vo-sites branch July 4, 2024 12:01
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants