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

CLI: added option to check a single import source or sync rule by name #2698

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

log1-c
Copy link

@log1-c log1-c commented Jan 24, 2023

Did a (possibly quick and dirty, as I'm not experienced in PHP) implementation of a new --name parameter to make it possible to check a single import source or sync rule with the cli command.

@cla-bot cla-bot bot added the cla/signed label Jan 24, 2023
@log1-c log1-c changed the title added option to check a single import source or sync rule by name CLI: added option to check a single import source or sync rule by name Jan 24, 2023
@log1-c
Copy link
Author

log1-c commented Jan 15, 2024

Issue birthday is nearing, so I'd like to ask for a current state.
Is there a plan to implement this (or something similar)?

PR Works:
image

boldly pinging @lippserd @Thomas-Gelf

@lippserd
Copy link
Member

@log1-c PR will be reviewed next week.

@lippserd lippserd self-requested a review October 25, 2024 12:02
@lippserd lippserd added the dev-call Issues and Pull Requests to be discussed at the Dev Call. label Oct 25, 2024
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

I think it makes sense to add an option to filter check targets by name. Since jobs also have a name, we could also support the filter here. With the current implementation, the combination of --name with --check deployment or --check jobs would fail in my opinion, as the corresponding filter parameter is missing for the functions. Anyway, I would like to have an implementation that provides roughly the following:

  • Support for multiple specification of --name. Wildcards should also be supported.
  • Analogous support for the specification of --exclude-name.
  • Trigger an error if --check deployment or --check config and --*name are specified.
  • Support filter arguments without having to specify --check so that it applies to sync, import and jobs.
  • Create a filter object from the specified filter arguments.
  • Apply the filter in the corresponding health check functions.

All in all, these changes are not trivial, so I would keep them to a minimum so that we get something done here:

  • Raise an error when --name is specified with --check deployment or --check config, i.e. $this->fail('--name is not supported with --check ...');
  • Support filtering of jobs.
  • Fix code style and indentation 😉.

@log1-c if you need any help with the changes requested, please let me know.

application/clicommands/HealthCommand.php Outdated Show resolved Hide resolved
application/clicommands/HealthCommand.php Show resolved Hide resolved
library/Director/Health.php Outdated Show resolved Hide resolved
library/Director/Health.php Show resolved Hide resolved
library/Director/Health.php Outdated Show resolved Hide resolved
library/Director/Health.php Outdated Show resolved Hide resolved
added basic error check to not use --name with deployment or config check
changed variable names
simplified if clauses
@log1-c
Copy link
Author

log1-c commented Nov 8, 2024

@lippserd
Thanks for the feedback.

I've done the requested changes regarding the variable naming and the if clauses.
I also added a basic check to raise an error if --name is used with --check deployment or --check config.
I have no idea what the whole filter stuff means or even could look like, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed dev-call Issues and Pull Requests to be discussed at the Dev Call.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants