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

LOG-5062: align validation with documentation #2394

Merged
merged 1 commit into from
Apr 19, 2024
Merged

LOG-5062: align validation with documentation #2394

merged 1 commit into from
Apr 19, 2024

Conversation

vparfonov
Copy link
Contributor

Description

This PR address to align validation feature with current documentation. During validation for ClusterLogging we will check is ClusterLogForwarder in the same namespace named the same exist if not, exception will throw.

/cc @cahartma @Clee2691
/assign @jcantrill

/cherry-pick

Links

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 11, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Mar 11, 2024 •
edited by openshift-ci bot

@vparfonov: This pull request references LOG-5062 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "4.8.0" version, but no target version was set.

In response to this:

Description

This PR address to align validation feature with current documentation. During validation for ClusterLogging we will check is ClusterLogForwarder in the same namespace named the same exist if not, exception will throw.

/cc @cahartma @Clee2691
/assign @jcantrill

/cherry-pick

Links

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@vparfonov
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2024
jcantrill
jcantrill reviewed Mar 11, 2024
View reviewed changes
internal/validations/clusterlogging/validate_clusterlogging_spec.go Outdated
if spec.Forwarder != nil || spec.LogStore != nil || spec.Curation != nil || spec.Visualization != nil {
return errors.NewValidationError("Only spec.collection is allowed when using multiple instances of ClusterLogForwarder: %s/%s", cl.Namespace, cl.Name)
key := types.NamespacedName{Name: cl.Name, Namespace: cl.Namespace}
clf := runtime.NewClusterLogForwarder(cl.Namespace, cl.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be anything other then "&logging.ClusterLogForwarder{}" since the "get" will fill in all the pieces if it is found using the key

internal/validations/clusterlogging/validate_clusterlogging_spec.go Show resolved Hide resolved
internal/validations/clusterlogging/validate_clusterlogging_spec.go Outdated
clf := runtime.NewClusterLogForwarder(cl.Namespace, cl.Name)
if err := k8sClient.Get(context.TODO(), key, clf); err != nil {
if apierrors.IsNotFound(err) {
return errors.NewValidationError("ClusterLogForwarder: %s/%s instance not found, ClusterLogging object in %q not named %q also "+
Copy link
Contributor

Choose a reason for hiding this comment

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

This should change based upon my first comment. As-is the message is really long and difficult to understand

internal/validations/clusterlogging/validate_clusterlogging_spec_test.go Outdated Show resolved Hide resolved
@vparfonov vparfonov force-pushed the log5062 branch 2 times, most recently from 089c68e to feb1974 Compare March 12, 2024 12:32
@vparfonov
Copy link
Contributor Author

/test functional

@vparfonov
Copy link
Contributor Author

/retest

jcantrill
jcantrill reviewed Mar 12, 2024
View reviewed changes
internal/validations/clusterlogging/validate_clusterlogging_spec.go Outdated
clf := &v1.ClusterLogForwarder{}
if err := k8sClient.Get(context.TODO(), key, clf); err != nil {
if apierrors.IsNotFound(err) {
return errors.NewValidationError("ClusterLogging: %s/%s instance requires to have a ClusterLogForwarder deployed in the same namespace named the same", cl.Namespace, cl.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider a custom error message depending upon the usecase; and CLF is not always required

  1. CL(openshift-logging/instance) only - This is a valid LEGACY usecase
  2. CL(openshift-logging/instance) & CLF(openshift-logging/instance) - This is a valid LEGACY usecase
  3. CL(ANY_NS/OTHER) - This is an invalid usecase
  4. CLF(ANY_NS/ANY_NAME) - This is a valid mCLF usecase
  5. CL(ANY_NS/ANY_NAME) && CLF(ANY_NS/ANY_NAME) - This is a valid mCLF usecase

ANY_NAME is any name other then "instance" in "openshift-logging"

It looks like this still does not support the expected outcome for #1. It may make more sense to refactor the method to try to load CLF first and then use a switch depending upon the outcome and ns/name combination:

clf, err := k8sClient.Get(NS, Name)
switch: {
case legacy CL only:
case legacy CL and CLF:
case mclf CL only:
case mclf CLF only:
case mCLF CLF and CL:
}

@vparfonov vparfonov changed the title LOG-5062: align validation with documentation WIP:LOG-5062: align validation with documentation Mar 12, 2024
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 12, 2024
@vparfonov
Copy link
Contributor Author

/retest

@vparfonov
Copy link
Contributor Author

/test e2e-target

1 similar comment
@vparfonov
Copy link
Contributor Author

/test e2e-target

@vparfonov
Copy link
Contributor Author

/retest

@vparfonov
Copy link
Contributor Author

/test e2e-target

1 similar comment
@vparfonov
Copy link
Contributor Author

/test e2e-target

@vparfonov vparfonov changed the title WIP:LOG-5062: align validation with documentation LOG-5062: align validation with documentation Mar 22, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 22, 2024
@jcantrill
Copy link
Contributor

@vparfonov one nit but we will keep this on hold until after the release

@vparfonov
LOG-5062: align validation function with documentation
a707bb8 
Signed-off-by: Vitalii Parfonov <vparfono@redhat.com>
@jcantrill
Copy link
Contributor

/approve
/lgtm

@jcantrill
Copy link
Contributor

/test ci/prow/e2e-target

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2024

@jcantrill: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test ci-index-cluster-logging-operator-bundle
  • /test e2e-target
  • /test functional-target
  • /test images
  • /test lint
  • /test unit

The following commands are available to trigger optional jobs:

  • /test e2e-ocp-target-minus-one
  • /test e2e-ocp-target-minus-two
  • /test functional

Use /test all to run all jobs.

In response to this:

/test ci/prow/e2e-target

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jcantrill
Copy link
Contributor

/test e2e-target

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 26, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, vparfonov

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2024
@vparfonov
Copy link
Contributor Author

/retest

@jcantrill
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2024
@jcantrill
Copy link
Contributor

/cherrypick release-5.9

@openshift-cherrypick-robot
Copy link

@jcantrill: once the present PR merges, I will cherry-pick it on top of release-5.9 in a new PR and assign it to you.

In response to this:

/cherrypick release-5.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 5b4f827 and 2 for PR HEAD a707bb8 in total

@jcantrill
Copy link
Contributor

/retest

1 similar comment
@jcantrill
Copy link
Contributor

/retest

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 16, 2024 •
edited

@vparfonov: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-target-minus-one a707bb8 link false /test e2e-ocp-target-minus-one

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vparfonov
Copy link
Contributor Author

/retest

@jcantrill
Copy link
Contributor

Manually merging. job was retired after PR was created

@jcantrill jcantrill merged commit 46b2541 into openshift:master Apr 19, 2024
8 of 10 checks passed
@openshift-cherrypick-robot
Copy link

@jcantrill: new pull request created: #2437

In response to this:

/cherrypick release-5.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Apr 19, 2024
Merged
@vparfonov vparfonov deleted the log5062 branch April 19, 2024 13:28
@vparfonov vparfonov mentioned this pull request Apr 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcantrill jcantrill jcantrill left review comments

@cahartma cahartma Awaiting requested review from cahartma

@Clee2691 Clee2691 Awaiting requested review from Clee2691

Assignees

@jcantrill jcantrill

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. release/6.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vparfonov @openshift-ci-robot @jcantrill @openshift-cherrypick-robot