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

Boostrapping Patch command #18

Closed
wants to merge 5 commits into from
Closed

Conversation

sfc-gh-japatel
Copy link
Contributor

#15

Notes:

  • Added API, helper class and patch command.
  • Tested this locally:
    • mvn compile quarkus:dev -Dquarkus.args='patch logger com.snowflake.kafka.connector -l INFO' -Dsuspend
  • Can add more comments and actual logic in next commits.
  • API for reference: Link

@gunnarmorling
Copy link
Collaborator

Thanks a lot @sfc-gh-japatel, that's a great start!

Can add more comments and actual logic in next commits.

Looking forward to it.

@aalmiray, for some reason this hasn't triggered a PR build run on GH Actions. Could anything have been broken by the latest release related work by any chance?

@gunnarmorling
Copy link
Collaborator

Ah, seems it's here where it went missing: 5885d1d#diff-5dbf1a803ecc13ff945a08ed3eb09149a83615e83f15320550af8e3a91976446L21-L23.

@aalmiray
Copy link
Contributor

Oops. I removed the PR trigger for regular build. My mistake.

@gunnarmorling
Copy link
Collaborator

No worries. Could you bring it back?

@aalmiray
Copy link
Contributor

Done 9598e8c

@gunnarmorling
Copy link
Collaborator

Thanks, @aalmiray!

- kcctl get loggers
- kcctl get loggers -p com.snowflake.kafka.connector
- kcctl patch logger com.snowflake.kafka.connector -l INFO
@gunnarmorling
Copy link
Collaborator

Hey @sfc-gh-japatel, thanks for the update! Could you run mvn license:format and commit the result (currently, the license headers are missing from the new files)? Could you also update the output of kcctl help in the README, showing the new command? Other than that, are you planning more changes, or is it good to go from your PoV?

Copy link
Collaborator

@gunnarmorling gunnarmorling left a comment

Choose a reason for hiding this comment

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

One more question inline.

@Inject
ConfigurationContext context;

@CommandLine.Option(names = { "-p", "--path" }, description = "Path of the connector", defaultValue = DEFAULT_PATH)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the "path" of a connector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Connector path is usually the root path of the connector based on the java package name. Or it can a single class name. I am ok to change the name if path doesnt seem correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @sfc-gh-japatel, thanks for the update! Could you run mvn license:format and commit the result (currently, the license headers are missing from the new files)? Could you also update the output of kcctl help in the README, showing the new command? Other than that, are you planning more changes, or is it good to go from your PoV?

Thanks @gunnarmorling
Ran the mvn command.
Added patch in help section of kcctl.

These are the only changes I am planning from loggers perspective.

@gunnarmorling
Copy link
Collaborator

Rebased and applied. Thanks a lot, @sfc-gh-japatel! Note I've split up get loggers and get logger into two commands, so to be consistent with the experience known from _kubetctl. Thanks again!

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

3 participants