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.

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

hooks: Add new hook SetupAndValidate() #1816

Merged
merged 3 commits into from
Jul 12, 2023
Merged

hooks: Add new hook SetupAndValidate() #1816

merged 3 commits into from
Jul 12, 2023

Conversation

doniacld
Copy link
Contributor

@doniacld doniacld commented Jul 10, 2023 •
edited

  • In order to extend the possibility to setup and initialise for extended functionalities, a new hook is needed SetupAndValidate.
  • Create a method on service (like for pod) NameWithoutNamespace() to remove the namespace prefix. It will enable to make call to the domain easily.
  • Realised that an error log was not properly formatted, just fixing it (not related to the PR).

@doniacld doniacld requested a review from a team as a code owner July 10, 2023 13:41
@doniacld doniacld requested a review from asauber July 10, 2023 13:41
@doniacld doniacld temporarily deployed to ci July 10, 2023 13:41 — with GitHub Actions Inactive
@doniacld doniacld force-pushed the pr/doniacld/hooks-setup-validate branch from efe6173 to dcff8d8 Compare July 10, 2023 13:46
@doniacld doniacld temporarily deployed to ci July 10, 2023 13:46 — with GitHub Actions Inactive
@doniacld doniacld force-pushed the pr/doniacld/hooks-setup-validate branch from dcff8d8 to 9abd429 Compare July 10, 2023 13:50
@doniacld doniacld temporarily deployed to ci July 10, 2023 13:50 — with GitHub Actions Inactive
@doniacld doniacld force-pushed the pr/doniacld/hooks-setup-validate branch from 9abd429 to 37d4983 Compare July 10, 2023 14:08
@doniacld doniacld temporarily deployed to ci July 10, 2023 14:08 — with GitHub Actions Inactive
@doniacld
hooks: Add a setup and validate a new hook for extras
1f2ed3a 
This allows updating cilium-cli externally with extended
initialisation, setup and validation.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld doniacld force-pushed the pr/doniacld/hooks-setup-validate branch from 37d4983 to fb5fcb1 Compare July 12, 2023 07:07
@doniacld doniacld temporarily deployed to ci July 12, 2023 07:07 — with GitHub Actions Inactive
tklauser
tklauser reviewed Jul 12, 2023
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM.

It looks like there are some unrelated formatting changes in the 2nd commit which are reverted in the 3rd commit. Might be worth to run gofmt at each commit to avoid these.

@doniacld @tklauser
peer: Add a method on service to return the name without the namespace
19657d9 
Add a new method NameWithoutNamespace on service entity to return only the
service name without the namespace prefix.  This allows to shoot the domain
address.

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@doniacld @tklauser
metrics: Fix format error log in validateMetric
42e777f 
Previously, the log error was not formatted  properly: "context timeout:
%!w(string=context deadline exceeded)", now it prints the error message a
string: "context timeout: context deadline exceeded".

Signed-off-by: Donia Chaiehloudj <donia.cld@isovalent.com>
@tklauser tklauser force-pushed the pr/doniacld/hooks-setup-validate branch from fb5fcb1 to 42e777f Compare July 12, 2023 11:44
@tklauser tklauser temporarily deployed to ci July 12, 2023 11:44 — with GitHub Actions Inactive
@tklauser
Copy link
Member

tklauser commented Jul 12, 2023 •
edited

It looks like there are some unrelated formatting changes in the 2nd commit which are reverted in the 3rd commit. Might be worth to run gofmt at each commit to avoid these.

FYI, I've force pushed to this branch to omit the formatting changes so we can merge this PR for v0.15.2.

@tklauser tklauser merged commit fdabbcd into cilium:main Jul 12, 2023
19 checks passed
@doniacld
Copy link
Contributor Author

@tklauser Thanks a lot for fixing my commits ❤️ and merging!

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

@tklauser tklauser tklauser approved these changes

@asauber asauber Awaiting requested review from asauber asauber is a code owner automatically assigned from cilium/cli

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@doniacld @tklauser