这是indexloc提供的服务，不要输入任何密码
Skip to content

Feature/crd humio view #257

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
Merged
merged 27 commits into from
Nov 13, 2020
Merged

Feature/crd humio view #257

merged 27 commits into from
Nov 13, 2020

Conversation

@mahlunar
Copy link
Contributor

@mahlunar mahlunar commented Nov 10, 2020

Fixes #207

Changes

Added HumioView CRD

Name and Connections can now be set.
Description is omitted for now as we would need an update to the view api

Controller

Added support for adding, updating and deleting views

Tests performed

I have run all tests via "make test".
I have also spun up a humio cluster via the hack tools and observed the changes to the humio views in the cluster.

Please let me know if anything is missing or should be changed.

@mahlunar mahlunar marked this pull request as ready for review November 11, 2020 14:23
@mahlunar
Copy link
Contributor Author

mahlunar commented Nov 11, 2020

I can see that the e2e tests are failing where running make test works.

I will gladly accept some hints on how to proceed 👍

@Crevil Crevil mentioned this pull request Nov 12, 2020
Closed
jswoods and others added 3 commits November 12, 2020 12:41
@jswoods
Add tolerations for humio pods
1993b77
Improve output on failed tests.
e07fe48 
Tests print out the latest `By()` so adding them different places makes
error messages much more clear.
Merge pull request #3 from lunarway/sync-upstream
0059181 
Sync upstream
@kaspernissen
Copy link

kaspernissen commented Nov 13, 2020

I just had a look at the failing test, it looks like it's failing because the Humio View test tries to create a repository that already exists. We should change the repositoryKey and repositoryToCreate resources to be unique so that it doesn't conflict with the repository test.

Humio Resources Controllers Humio View 
  Should handle view correctly
  /home/runner/work/humio-operator/humio-operator/controllers/humioresources_controller_test.go:343
STEP: Creating a shared humio cluster if it doesn't already exist
STEP: Creating the repository successfully

• Failure [0.008 seconds]
Humio Resources Controllers
/home/runner/work/humio-operator/humio-operator/controllers/humioresources_controller_test.go:35
  Humio View
  /home/runner/work/humio-operator/humio-operator/controllers/humioresources_controller_test.go:342
    Should handle view correctly [It]
    /home/runner/work/humio-operator/humio-operator/controllers/humioresources_controller_test.go:343

    Expected success, but got an error:
        <*errors.StatusError | 0xc000f4aa00>: {
            ErrStatus: {
                TypeMeta: {Kind: "", APIVersion: ""},
                ListMeta: {
                    SelfLink: "",
                    ResourceVersion: "",
                    Continue: "",
                    RemainingItemCount: nil,
                },
                Status: "Failure",
                Message: "humiorepositories.core.humio.com \"humiorepository\" already exists",
                Reason: "AlreadyExists",
                Details: {
                    Name: "humiorepository",
                    Group: "core.humio.com",
                    Kind: "humiorepositories",
                    UID: "",
                    Causes: nil,
                    RetryAfterSeconds: 0,
                },
                Code: 409,
            },
        }
        humiorepositories.core.humio.com "humiorepository" already exists

    /home/runner/work/humio-operator/humio-operator/controllers/humioresources_controller_test.go:389

@Crevil
Copy link
Contributor

Crevil commented Nov 13, 2020

Ah, that should be an easy fix. Will you go ahead or should I @kaspernissen ?

@kaspernissen
Copy link

kaspernissen commented Nov 13, 2020

I think I might have it. make test succeeds locally (it also did in the initial version, so no guarantee), let's see if e2e succeeds here as well 🙏

@kaspernissen
Copy link

kaspernissen commented Nov 13, 2020

Unfortunately, didn't fix the failing test. We found the issue, which seems to be an addition in the cli package, that now results in the expectation to not match; https://github.com/humio/cli/blame/dea38288df5de0e5e4c52a049207b7322ba1bc4c/api/repositories.go#L15

@mahlunar is working on a fix.

@mahlunar
Copy link
Contributor Author

mahlunar commented Nov 13, 2020

Fixed. The Repository Id was the culprit which failed on assertions. I have added a new struct in the test file that excludes the Id field. Let me know if this is ok.

@mahlunar mahlunar changed the title [WIP] Feature/crd humio view Feature/crd humio view Nov 13, 2020
SaaldjorMike
SaaldjorMike reviewed Nov 13, 2020
View reviewed changes
Copy link
Contributor

@SaaldjorMike SaaldjorMike left a comment

Choose a reason for hiding this comment

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

LGTM. The only comment I have is that you need to make sure to incorporate the latest changes on master and regenerate the CRD manifests. That is because we are now using a slightly newer controller-gen version, and I'd much prefer if we have all CRD files generated based on the same version.

To generate the CRD's again, you have to remove the old controller-gen binary and then generate the CRD manifests. Generating CRD manifests will automatically grab the controller-gen version but currently it does not handle updates to it that well. So this would be what you need to do after incorporating the latest changes on master:

rm ~/go/bin/controller-gen
make manifests

Then add the updated CRD files to this PR. After that we should be ready to go.

Thank you so much for this contribution, it is much appreciated!

Kasper Nissen and others added 17 commits November 13, 2020 13:41
SaaldjorMike
SaaldjorMike approved these changes Nov 13, 2020
View reviewed changes
Copy link
Contributor

@SaaldjorMike SaaldjorMike left a comment

Choose a reason for hiding this comment

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

LGTM.

@mahlunar
Copy link
Contributor Author

mahlunar commented Nov 13, 2020

Rebase done and updated manifests.

@jswoods jswoods merged commit 1aee196 into humio:master Nov 13, 2020
@mahlunar mahlunar deleted the feature/CRD-HumioView branch November 13, 2020 17:50
triceras pushed a commit that referenced this pull request Mar 12, 2025
@mahlunar
Feature/crd humio view (#257)
0efe4c1 
* Add support for Humio Views
* Bump humio/cli version to v0.28.1

Co-authored-by: Kasper Nissen <kni@lunar.app>
Co-authored-by: Bjørn <bso@lunar.app>
@bowens-stripe bowens-stripe mentioned this pull request Apr 23, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jswoods jswoods jswoods approved these changes

+1 more reviewer

@SaaldjorMike SaaldjorMike SaaldjorMike approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: CRD for HumioViews

5 participants

@mahlunar @kaspernissen @Crevil @jswoods @SaaldjorMike