KOTS reporting to replicated-app #4202

FourSigma · 2023-12-05T15:26:14Z

What this PR does / why we need it:

Injects relevant header metadata when KOTS reaches out to Replicated.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Steps to reproduce

Does this PR introduce a user-facing change?

NONE

Does this PR require documentation?

CLAassistant · 2023-12-05T15:26:20Z

All committers have signed the CLA.

divolgin · 2023-12-05T18:54:51Z

pkg/replicatedapp/api.go

+	}
+
+	if l != nil {
+		reportingInfo := reporting.GetReportingInfoByAppSlug(l.Spec.AppSlug)


The app slug is generated and won't always match the one in the license. I think you will need to use ListInstalledApps and then GetLatestLicenseForApp and then just call GetReportingInfo with the right app ID

FourSigma · 2023-12-13T18:54:03Z

@sgalsaleh or @cbodonnell Would love you thoughts on how to globally mock a store.Store for failing unit test. I can't find a good pathway to dependency injection neatly.

Basically, the test is failing because of a callstore.GetStore() which is nil.

cbodonnell · 2023-12-13T20:48:49Z

pkg/replicatedapp/api.go

@@ -149,6 +180,14 @@ func getApplicationMetadataFromHost(host string, endpoint string, upstream *url.
 		return nil, errors.Wrap(err, "failed to call newrequest")
 	}

+	appID, err := store.GetStore().GetAppIDFromSlug(r.AppSlug)


It looks like store.GetStore() is returning nil here as well based on the failing e2e jobs. This function is called in the context of the KOTS CLI, which is why it does not have direct access to the store.

So store.GetStore(), might not be returning nil here as I had thought above, but this would get called at a time where rqlite has not yet been deployed by KOTS and is the cause of the failing tests. Looking at the CI jobs, the kots install fails before any of the components are deployed. My suggestion would be to remove reporting from the metadata request here since it happens before a license is even installed. This initial metadata request is unauthenticated and is just used to apply branding to the initial screens.

cbodonnell · 2023-12-13T20:51:19Z

@sgalsaleh or @cbodonnell Would love you thoughts on how to globally mock a store.Store for failing unit test. I can't find a good pathway to dependency injection neatly.

Basically, the test is failing because of a callstore.GetStore() which is nil.

In the SDK, we have this function, which allows setting the global store. Something like that would allow you to set and defer unset. In general, we've run into this issue a lot wrt the store dependency and it often requires a bit of refactoring to have functions accept the store.Store interface so we can unit test appropriately.

FourSigma · 2023-12-15T19:20:24Z

@sgalsaleh or @cbodonnell Would love you thoughts on how to globally mock a store.Store for failing unit test. I can't find a good pathway to dependency injection neatly.
Basically, the test is failing because of a callstore.GetStore() which is nil.

In the SDK, we have this function, which allows setting the global store. Something like that would allow you to set and defer unset. In general, we've run into this issue a lot wrt the store dependency and it often requires a bit of refactoring to have functions accept the store.Store interface so we can unit test appropriately.

@cbodonnell
Fixed this issue and the ci-test passes.

However, a large number of integration tests are failing with the error error connecting to rqlite: url specified is impossibly short. What the remediation for this particular error case?

cbodonnell · 2023-12-15T20:28:33Z

@sgalsaleh or @cbodonnell Would love you thoughts on how to globally mock a store.Store for failing unit test. I can't find a good pathway to dependency injection neatly.
Basically, the test is failing because of a callstore.GetStore() which is nil.

In the SDK, we have this function, which allows setting the global store. Something like that would allow you to set and defer unset. In general, we've run into this issue a lot wrt the store dependency and it often requires a bit of refactoring to have functions accept the store.Store interface so we can unit test appropriately.

@cbodonnell Fixed this issue and the ci-test passes.

However, a large number of integration tests are failing with the error error connecting to rqlite: url specified is impossibly short. What the remediation for this particular error case?

@FourSigma I just left a follow-up comment here with my thoughts. Hopefully that is all that is causing the tests to trip up since the panic originates from the call to getApplicationMetadataFromHost (link).

FourSigma · 2023-12-15T20:40:51Z

@sgalsaleh or @cbodonnell Would love you thoughts on how to globally mock a store.Store for failing unit test. I can't find a good pathway to dependency injection neatly.
Basically, the test is failing because of a callstore.GetStore() which is nil.

In the SDK, we have this function, which allows setting the global store. Something like that would allow you to set and defer unset. In general, we've run into this issue a lot wrt the store dependency and it often requires a bit of refactoring to have functions accept the store.Store interface so we can unit test appropriately.

@cbodonnell Fixed this issue and the ci-test passes.
However, a large number of integration tests are failing with the error error connecting to rqlite: url specified is impossibly short. What the remediation for this particular error case?

@FourSigma I just left a follow-up comment here with my thoughts. Hopefully that is all that is causing the tests to trip up since the panic originates from the call to getApplicationMetadataFromHost (link).

Makes sense @cbodonnell . I will make the change.

FourSigma · 2023-12-18T18:19:33Z

@cbodonnell Happy Monday. Looks like we have some failures on validate-kots-pull. Since you are bit more familiar with this pathway, do you have some suggestions to get these test passing without too much refactoring?

cbodonnell · 2023-12-18T20:24:12Z

@cbodonnell Happy Monday. Looks like we have some failures on validate-kots-pull. Since you are bit more familiar with this pathway, do you have some suggestions to get these test passing without too much refactoring?

the kots pull command also runs from the CLI before any resources are deployed, so the underlying failure reason is the same as what we saw for kots install. if we need to avoid refactoring, one possibility could be to log (maybe just at the debug level) and move on without attempting to add headers if the database call fails. this way it can still function if we do have a database context available, but will not if it's running from the CLI only. thoughts?

FourSigma · 2023-12-18T20:57:13Z

@cbodonnell Happy Monday. Looks like we have some failures on validate-kots-pull. Since you are bit more familiar with this pathway, do you have some suggestions to get these test passing without too much refactoring?

the kots pull command also runs from the CLI before any resources are deployed, so the underlying failure reason is the same as what we saw for kots install. if we need to avoid refactoring, one possibility could be to log (maybe just at the debug level) and move on without attempting to add headers if the database call fails. this way it can still function if we do have a database context available, but will not if it's running from the CLI only. thoughts?

I would be okay for this approach esp. if it just limited to the reporting portion of KOTS.

FourSigma · 2023-12-19T18:12:36Z

@cbodonnell Looks like there is a validate-gitops (kind, v1.28.0) failure. Do you have some additional context on this test?

cbodonnell · 2023-12-19T18:50:47Z

@cbodonnell Looks like there is a validate-gitops (kind, v1.28.0) failure. Do you have some additional context on this test?

@FourSigma It looks like the testim branch was out-of-date since that test was added somewhat recently. I've merged in changes from the base branch and re-run the test.

FourSigma · 2023-12-19T22:57:18Z

@cbodonnell Looks like we are now failing on the following tests:

build-test / validate-smoke-test (aks, 1.25, alpha) (pull_request)
build-test / validate-backup-and-restore (eks, 1.27, stable) (pull_request)

cbodonnell · 2023-12-20T13:40:47Z

@cbodonnell Looks like we are now failing on the following tests:

build-test / validate-smoke-test (aks, 1.25, alpha) (pull_request) build-test / validate-backup-and-restore (eks, 1.27, stable) (pull_request)

@FourSigma those both looked to be flaky failures since a re-run passed. Merged this one.

FourSigma · 2023-12-20T15:25:59Z

@cbodonnell Looks like we are now failing on the following tests:
build-test / validate-smoke-test (aks, 1.25, alpha) (pull_request) build-test / validate-backup-and-restore (eks, 1.27, stable) (pull_request)

@FourSigma those both looked to be flaky failures since a re-run passed. Merged this one.

Thanks @cbodonnell!

divolgin reviewed Dec 5, 2023

View reviewed changes

FourSigma force-pushed the siva/sc-93559/wrong-data-in-field-customer-last-active branch from 6923957 to 730cfeb Compare December 6, 2023 15:44

FourSigma requested review from sgalsaleh and cbodonnell December 13, 2023 18:50

cbodonnell reviewed Dec 13, 2023

View reviewed changes

FourSigma added the type::feature New feature or request label Dec 15, 2023

FourSigma force-pushed the siva/sc-93559/wrong-data-in-field-customer-last-active branch 2 times, most recently from 19dcd62 to f1c319c Compare December 15, 2023 18:08

FourSigma requested a review from cbodonnell December 15, 2023 19:44

FourSigma force-pushed the siva/sc-93559/wrong-data-in-field-customer-last-active branch 3 times, most recently from 1160566 to b1263f4 Compare December 18, 2023 22:42

cbodonnell approved these changes Dec 19, 2023

View reviewed changes

Siva Manivannan added 6 commits December 19, 2023 12:56

update reporting

e5e9379

add function to get reporting info by app slug

4c68232

slug is not consistent use app id

5f38bb5

set store in kots for testing

7fd75a3

Empty-Commit

6cc6c7d

remove reporting from getApplicationMetadataFromHost function

d362386

store not initialized error

b29c25b

FourSigma force-pushed the siva/sc-93559/wrong-data-in-field-customer-last-active branch from b1263f4 to b29c25b Compare December 19, 2023 18:57

cbodonnell merged commit 950a658 into main Dec 20, 2023
175 checks passed

cbodonnell deleted the siva/sc-93559/wrong-data-in-field-customer-last-active branch December 20, 2023 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KOTS reporting to replicated-app #4202

KOTS reporting to replicated-app #4202

FourSigma commented Dec 5, 2023 •

edited

Loading

CLAassistant commented Dec 5, 2023 •

edited

Loading

divolgin Dec 5, 2023

FourSigma commented Dec 13, 2023

cbodonnell Dec 13, 2023

cbodonnell Dec 15, 2023

cbodonnell commented Dec 13, 2023

FourSigma commented Dec 15, 2023 •

edited

Loading

cbodonnell commented Dec 15, 2023

FourSigma commented Dec 15, 2023

FourSigma commented Dec 18, 2023

cbodonnell commented Dec 18, 2023 •

edited by FourSigma

Loading

FourSigma commented Dec 18, 2023

FourSigma commented Dec 19, 2023

cbodonnell commented Dec 19, 2023

FourSigma commented Dec 19, 2023

cbodonnell commented Dec 20, 2023

FourSigma commented Dec 20, 2023

KOTS reporting to replicated-app #4202

KOTS reporting to replicated-app #4202

Conversation

FourSigma commented Dec 5, 2023 • edited Loading

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Steps to reproduce

Does this PR introduce a user-facing change?

Does this PR require documentation?

CLAassistant commented Dec 5, 2023 • edited Loading

divolgin Dec 5, 2023

Choose a reason for hiding this comment

FourSigma commented Dec 13, 2023

cbodonnell Dec 13, 2023

Choose a reason for hiding this comment

cbodonnell Dec 15, 2023

Choose a reason for hiding this comment

cbodonnell commented Dec 13, 2023

FourSigma commented Dec 15, 2023 • edited Loading

cbodonnell commented Dec 15, 2023

FourSigma commented Dec 15, 2023

FourSigma commented Dec 18, 2023

cbodonnell commented Dec 18, 2023 • edited by FourSigma Loading

FourSigma commented Dec 18, 2023

FourSigma commented Dec 19, 2023

cbodonnell commented Dec 19, 2023

FourSigma commented Dec 19, 2023

cbodonnell commented Dec 20, 2023

FourSigma commented Dec 20, 2023

FourSigma commented Dec 5, 2023 •

edited

Loading

CLAassistant commented Dec 5, 2023 •

edited

Loading

FourSigma commented Dec 15, 2023 •

edited

Loading

cbodonnell commented Dec 18, 2023 •

edited by FourSigma

Loading