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

Change BulkCreate ApplicationAuthentication logic to work with DB-backed DAO #193

Merged
merged 2 commits into from Apr 1, 2022
Merged

Change BulkCreate ApplicationAuthentication logic to work with DB-backed DAO #193

merged 2 commits into from Apr 1, 2022

Conversation

lindgrenj6
Copy link
Contributor

@lindgrenj6 lindgrenj6 commented Mar 31, 2022

BulkCreate wasn't working for applicationAuthentications before mostly because of our DB schema being not-correct for the vault backend.

In the future we're going to need to relax the constraints and change that table's layout a bit to fit both options. But today is not that day since we do not have a vault.

Copy link
Member

@MikelAlejoBR MikelAlejoBR left a comment

Choose a reason for hiding this comment

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

Hang on a minute. I was curious about the following change:

- VaultPath string `json:"vault_path"`
+ VaultPath string `json:"vault_path" gorm:"-"`

Since I remember I had to add it back because some tests are failing. And even though the CI says everything is right... If I issue the go test ./... -integration command locally, I get a test failure in the DAO package....

--- FAIL: TestDeleteApplicationAuthentication (0.19s)                                                                                                                                                                                       
    application_authentication_dao_test.go:51: incorrect applicationAuthentication deleted. Want "complex uuid" in the authenticationUid field, got ""

And a panic from a dereference error in the status listener package. Can you please run the tests locally too with the above command and confirm that it's not just me?

In the meantime I'm going to investigate what's failing.

@MikelAlejoBR
Copy link
Member

Since I remember I had to add it back because some tests are failing. And even though the CI says everything is right... If I issue the go test ./... -integration command locally, I get a test failure in the DAO package....

--- FAIL: TestDeleteApplicationAuthentication (0.19s)                                                                                                                                                                                       
    application_authentication_dao_test.go:51: incorrect applicationAuthentication deleted. Want "complex uuid" in the authenticationUid field, got ""

Regarding this error, it corresponds to the following code block:

{
want := applicationAuthentication.AuthenticationUID
got := deletedApplicationAuthentication.AuthenticationUID
if want != got {
t.Errorf(`incorrect applicationAuthentication deleted. Want "%s" in the authenticationUid field, got "%s"`, want, got)
}
}

It seems that when I wrote the test, I was using the UUID as an extra check to see if I was fetching the correct application authentication. I guess we can safely delete that code block.

@MikelAlejoBR
Copy link
Member

And a panic from a dereference error in the status listener package. Can you please run the tests locally too with the above command and confirm that it's not just me?

Regarding this one, I've been reverting the main branch locally until the tests stopped failing, and it seems that the bug was introduced in the commit bfa0816, after merging the PR #168 .

I've opened a ticket so that we don't forget to change the CI command so that it includes all the tests: https://issues.redhat.com/browse/RHCLOUD-18699


Since it's unrelated to this PR I think it is safe merging this PR after the DAO test change.

@lpichler
Copy link
Contributor

lpichler commented Apr 1, 2022

/retest

@lindgrenj6
Copy link
Contributor Author

I went through and removed all refs to AuthenticationUID and VaultPath since those aren't actually in the DB. We can re-add them in if/when we get a Vault.

@lpichler lpichler merged commit a700bdf into RedHatInsights:main Apr 1, 2022
@lindgrenj6 lindgrenj6 deleted the bulk-create-db-fixes branch April 1, 2022 16:35
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.

None yet

3 participants