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

🌱 Clean up logging and error handling in the webhooks #1427

Merged
merged 1 commit into from
Nov 7, 2023
Merged

🌱 Clean up logging and error handling in the webhooks #1427

merged 1 commit into from
Nov 7, 2023

Conversation

dtantsur
Copy link
Member

@dtantsur dtantsur commented Nov 3, 2023

Logging:

  • Rename the webhook loggers for consistency and easier grepping
  • Remove redundant logging from the validation code

Errors:

  • Provide wrapped errors in a consistent fashion
  • Do not refer to internal constant names in user-visible strings
  • Provide allowed values when saying that something is not allowed
  • Use the stdlib error routines instead of the deprecated pkg/errors.
  • Do not shadow the errors package name by local variables

@metal3-io-bot metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 3, 2023
@dtantsur dtantsur changed the title 🌱 Clean up logging and error handling in the BMH webhook 🌱 Clean up logging and error handling in the webhooks Nov 3, 2023
@dtantsur
Copy link
Member Author

dtantsur commented Nov 3, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

1 similar comment
@dtantsur
Copy link
Member Author

dtantsur commented Nov 3, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

zaneb
zaneb reviewed Nov 5, 2023
View reviewed changes
Copy link
Member

@zaneb zaneb left a comment

Choose a reason for hiding this comment

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

/approve

apis/metal3.io/v1alpha1/baremetalhost_webhook.go Outdated Show resolved Hide resolved
apis/metal3.io/v1alpha1/bmceventsubscription_webhook.go Outdated Show resolved Hide resolved
apis/metal3.io/v1alpha1/bmceventsubscription_validation.go Show resolved Hide resolved
@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zaneb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2023
zaneb
zaneb reviewed Nov 6, 2023
View reviewed changes
apis/metal3.io/v1alpha1/bmceventsubscription_validation.go
var errs []error

if s.Spec.HostName == "" {
errs = append(errs, fmt.Errorf("HostName cannot be empty"))
errs = append(errs, errors.New("hostName cannot be empty"))
Copy link
Member

Choose a reason for hiding this comment

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

We should be validating this client-side, you don't need a webhook just to make sure a field is not empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but I'd rather keep this PR refactoring-only

@dtantsur
Clean up logging and error handling in the webhooks
a72ffa6 
Logging:
- Rename the webhook loggers for consistency and easier grepping
- Remove redundant logging from the validation code

Errors:
- Provide wrapped errors in a consistent fashion
- Do not refer to internal constant names in user-visible strings
- Match field names to those in JSON (i.e. camelCase)
- Provide allowed values when saying that something is not allowed
- Use the stdlib error routines instead of the deprecated pkg/errors.
- Do not shadow the errors package name by local variables
@zaneb
Copy link
Member

zaneb commented Nov 6, 2023

/test-centos-e2e-integration-main
/test-ubuntu-integration-main

@elfosardo
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 7, 2023
@metal3-io-bot metal3-io-bot merged commit 21c9dfd into metal3-io:main Nov 7, 2023
13 checks passed
@dtantsur dtantsur deleted the validate-errors branch November 7, 2023 09:49
zaneb added a commit to zaneb/bmo-log-parse that referenced this pull request Nov 7, 2023
@zaneb
Recognise new webhooks logger
9b12e97 
metal3-io/baremetal-operator#1427 switches both
webhooks to use the name 'webhooks' for the logger.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zaneb zaneb zaneb left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtantsur @metal3-io-bot @zaneb @elfosardo