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

Fix action store -> state store migration tests #4235

Conversation

AndersonQ
Copy link
Member

@AndersonQ AndersonQ commented Feb 9, 2024

What does this PR do?

Fixes the state store migration tests so they actually run and skip them for darwin.
It also changes the migration test to use a golden file when testing the migration from the old action store to the new state store.
Finally, it removes the withFile function as its miss usage caused the store migration tests to not actually run.

Why is it important?

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in ./changelog/fragments using the changelog tool
  • [ ] I have added an integration test or an E2E test

Related issues

Questions to ask yourself

  • How are we going to support this in production?
  • How are we going to measure its adoption?
  • How are we going to debug this?
  • What are the metrics I should take care of?
  • ...

@AndersonQ AndersonQ self-assigned this Feb 9, 2024
@AndersonQ AndersonQ requested a review from a team as a code owner February 9, 2024 16:15
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

@AndersonQ AndersonQ changed the base branch from main to bugfix/3912-borken-stat-store February 9, 2024 16:19
@AndersonQ AndersonQ marked this pull request as ready for review February 9, 2024 16:19
@AndersonQ AndersonQ force-pushed the 3912-add-state-store-migration-tests branch from 8d69a4e to ea73ae9 Compare February 12, 2024 10:24
@AndersonQ AndersonQ marked this pull request as draft February 12, 2024 11:10
@AndersonQ AndersonQ changed the title Use golden file for store migration test Fix store migration tests Feb 12, 2024
Copy link

Quality Gate failed Quality Gate failed

Failed conditions

0.0% 0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

@AndersonQ AndersonQ marked this pull request as ready for review February 12, 2024 15:35
Copy link
Member

@pchila pchila left a comment

Choose a reason for hiding this comment

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

Code looks reasonable, I am a bit surprised by the 0% new code coverage... are the tests actually running?

Moreover, I think that there's a bit of overlap with a bugfix I merged last year where I was moving unencrypted config to the new encrypted diskstores (as the agent was "forgetting" fleet config because fleet.enc was not found), so maybe we need to clean up a bit. Here's where the bugfix is located

// Migrate .yml files if the corresponding .enc does not exist
// the encrypted config does not exist but the unencrypted file does
err = migration.MigrateToEncryptedConfig(ctx, l, paths.AgentConfigYmlFile(), paths.AgentConfigFile())
if err != nil {
return errors.New(err, "error migrating fleet config")
}
// the encrypted state does not exist but the unencrypted file does
err = migration.MigrateToEncryptedConfig(ctx, l, paths.AgentStateStoreYmlFile(), paths.AgentStateStoreFile())
if err != nil {
return errors.New(err, "error migrating agent state")
}

@AndersonQ
Copy link
Member Author

I am a bit surprised by the 0% new code coverage... are the tests actually running?

the new code is test code, which isn't covered by tests. I don't think it's calculating the overall coverage.
Let me calculate it

@AndersonQ
Copy link
Member Author

Quality Gate failed Quality Gate failed

Failed conditions

0.0% 0.0% Coverage on New Code (required ≥ 40%)

See analysis details on SonarQube

As Sonarqube looks for coverage only on NEW code and all the new code is test code, it isn't calculating the overall code coverage increase. Here it's:

  • main:
 ~/devel/github.com/elastic/elastic-agent/internal/pkg/agent/storage/store   main  16:22:29                                                                                     1.223s
❯ go test -cover   
PASS
coverage: 61.2% of statements
ok  	github.com/elastic/elastic-agent/internal/pkg/agent/storage/store	0.481s
  • bugfix/3912-borken-stat-store (the PR's base branch)
 ~/devel/github.com/elastic/elastic-agent/internal/pkg/agent/storage/store   bugfix/3912-borken-stat-store  16:22:43                                                            0.038s
❯ go test -cover                            
PASS
coverage: 61.2% of statements
ok  	github.com/elastic/elastic-agent/internal/pkg/agent/storage/store	0.476s
  • 3912-add-state-store-migration-tests: the PR itself
 ~/devel/github.com/elastic/elastic-agent/internal/pkg/agent/storage/store   3912-add-sta…ration-tests  16:22:53                                                                0.024s
❯ go test -cover                                   
PASS
coverage: 75.2% of statements
ok  	github.com/elastic/elastic-agent/internal/pkg/agent/storage/store	0.523s

@pchila here it is, a 14% increase in code coverage

@AndersonQ AndersonQ requested a review from pchila February 13, 2024 15:29
@AndersonQ AndersonQ changed the title Fix store migration tests Fix action store -> state store migration tests Feb 14, 2024
@AndersonQ
Copy link
Member Author

AndersonQ commented Feb 14, 2024

Moreover, I think that there's a bit of overlap with a bugfix I merged last year where I was moving unencrypted config to the new encrypted diskstores (as the agent was "forgetting" fleet config because fleet.enc was not found), so maybe we need to clean up a bit. Here's where the bugfix is located

// Migrate .yml files if the corresponding .enc does not exist
// the encrypted config does not exist but the unencrypted file does
err = migration.MigrateToEncryptedConfig(ctx, l, paths.AgentConfigYmlFile(), paths.AgentConfigFile())
if err != nil {
return errors.New(err, "error migrating fleet config")
}
// the encrypted state does not exist but the unencrypted file does
err = migration.MigrateToEncryptedConfig(ctx, l, paths.AgentStateStoreYmlFile(), paths.AgentStateStoreFile())
if err != nil {
return errors.New(err, "error migrating agent state")
}

@pchila, good point, it might be messing the store migration. I added a TODO on my currently WIP PR: 4926df0

@AndersonQ AndersonQ merged commit db6c7a6 into elastic:bugfix/3912-borken-stat-store Feb 15, 2024
8 of 9 checks passed
@AndersonQ AndersonQ deleted the 3912-add-state-store-migration-tests branch March 7, 2024 10:00
@AndersonQ AndersonQ mentioned this pull request Apr 2, 2024
4 tasks
AndersonQ added a commit that referenced this pull request Jun 27, 2024
Fix action store -> state store migration tests (#4235)
* use golden file for store migration test
* remove withFile function
* migrate take in a storage.Store instead of the storage's path. It's needed so we can set the encrypted store vault's path

refactor state store (#4253)
It modifies the state store API to match the current needs.

update action model to match fleet-server schema (#4240)
* simplify fleetapi.Actions.UnmarshalJSON
* add test to ensure the state store is correctly loaded from disk
* skip state store migration tests, they will be fixes on a follow-up PR as part of #3912

add migrations for action and state stores (#4305)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants