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

Feat crd autodiscovery #57

Merged
merged 32 commits into from
May 9, 2024
Merged

Feat crd autodiscovery #57

merged 32 commits into from
May 9, 2024

Conversation

danielsinai
Copy link
Contributor

@danielsinai danielsinai commented Apr 21, 2024

Description

What - Added auto discovery for CRDs & XRDs into the Portal embedded in the K8s exporter
Why - In order to allow seamless integration to k8s API
How - introduced a new discoverResourceDefinitionPattern which automatically creates the resource mapping using a new '*' key. As well as creating the blueprint + actions out of the vox

Type of change

  • New feature (non-breaking change which adds functionality)

@danielsinai danielsinai mentioned this pull request Apr 21, 2024
1 task
Copy link
Contributor

@talsabagport talsabagport left a comment

Choose a reason for hiding this comment

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

Amazing work, left few comments 🥇

pkg/handlers/controllers.go Outdated Show resolved Hide resolved
pkg/crd/crd.go Outdated Show resolved Hide resolved
pkg/crd/crd.go Outdated Show resolved Hide resolved
pkg/crd/crd.go Outdated Show resolved Hide resolved
Copy link
Contributor

@talsabagport talsabagport left a comment

Choose a reason for hiding this comment

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

🚀 Left some comments

@@ -118,7 +100,8 @@ func Test_InitIntegration_InitDefaults_CreateDefaultResources_False(t *testing.T
_, err := integration.GetIntegration(f.portClient, f.stateKey)
assert.Nil(t, err)

checkResourcesDoesNotExist(f, []string{"workload", "namespace", "cluster"}, []string{"workload_overview_dashboard", "availability_scorecard_dashboard"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just extracted it to the utils because we have common logic outside.

"github.com/stretchr/testify/assert"
)

func CheckResourcesExist(portClient *cli.PortClient, t *testing.T, blueprints []string, pages []string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can have a base function and call it twice, with a flag of "exists". The functions are exactly the same except assert.Nil vs assert.NotNil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, let me work it through

pkg/crd/crd.go Outdated Show resolved Hide resolved
pkg/crd/crd.go Outdated Show resolved Hide resolved
func CheckResourcesDoesNotExist(portClient *cli.PortClient, t *testing.T, blueprints []string, pages []string) {
for _, bp := range blueprints {
_, err := blueprint.GetBlueprint(portClient, bp)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why if err != nil (means that the blueprint does not exists), it tries to delete it? Shouldn't be the opposite? The same with pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's complicated I actually talked it out with @yairsimantov20 to understand, seems like this logic deleted the entities just in case it doesn't gets an error, but fails the test anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh just took another look on this, and yea I get what you mean. Let me fix it :)

portClient: portClient,
apiextensionClient: &apiExtensionsFakeClient,
portConfig: &port.IntegrationAppConfig{
CRDSToDiscover: "true",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why always true? Not worth to test it as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will add a test to cover it as well 👍

@danielsinai danielsinai merged commit 1b08c0f into main May 9, 2024
1 check passed
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.

2 participants