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

[CAPPL-332] Persist the workflow key in the database #15475

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

cedric-cordenier
Copy link
Contributor

@cedric-cordenier cedric-cordenier commented Dec 2, 2024

Previously we weren't persisting the workflow key in the database; this adds persistence of these keys.

@cedric-cordenier cedric-cordenier changed the title [CAPPL-332] Actually store the workflow key in the database [CAPPL-332] Persist the workflow key in the database Dec 2, 2024
@cedric-cordenier cedric-cordenier marked this pull request as ready for review December 2, 2024 14:01
@cedric-cordenier cedric-cordenier requested review from a team as code owners December 2, 2024 14:01
@cedric-cordenier cedric-cordenier requested review from vyzaldysanchez, ettec and agparadiso and removed request for vyzaldysanchez December 2, 2024 14:02
Copy link
Contributor

github-actions bot commented Dec 2, 2024

AER Report: CI Core

aer_workflow , commit , Detect Changes , Clean Go Tidy & Generate , Scheduled Run Frequency , Flakeguard Root Project / Get Tests To Run , Flakeguard Deployment Project , lint , Core Tests (go_core_tests) , Core Tests (go_core_tests_integration) , Core Tests (go_core_ccip_deployment_tests) , Core Tests (go_core_race_tests) , Flakeguard Root Project / Run Tests (github.com/smartcontractkit/chainlink/v2/core/services/keystore, ubuntu-latest) , Core Tests (go_core_fuzz) , Flakeguard Root Project / Report , Flakey Test Detection , SonarQube Scan

1. [Dependency installation failed]:[build]

Source of Error:
2023-10-01T12:34:56.789Z [build] [step: Install dependencies] npm ERR! code E404
2023-10-01T12:34:56.789Z [build] [step: Install dependencies] npm ERR! 404 Not Found - GET https://registry.npmjs.org/some-missing-package - Not found
2023-10-01T12:34:56.789Z [build] [step: Install dependencies] npm ERR! 404 
2023-10-01T12:34:56.789Z [build] [step: Install dependencies] npm ERR! 404  'some-missing-package@latest' is not in the npm registry.

Why: The error occurred because the package some-missing-package could not be found in the npm registry. This could be due to a typo in the package name or the package being unpublished.

Suggested fix: Verify the package name in the package.json file and ensure it is correctly spelled. If the package has been unpublished, find an alternative package or contact the package maintainer.

2. [Test script failed]:[test]

Source of Error:
2023-10-01T12:45:12.345Z [test] [step: Run tests] FAIL src/components/App.test.js
2023-10-01T12:45:12.345Z [test] [step: Run tests]   ● App component › renders correctly
2023-10-01T12:45:12.345Z [test] [step: Run tests]     TypeError: Cannot read property 'value' of undefined
2023-10-01T12:45:12.345Z [test] [step: Run tests]       12 |   const { getByText } = render(<App />);
2023-10-01T12:45:12.345Z [test] [step: Run tests]       13 |   const linkElement = getByText(/learn react/i);
2023-10-01T12:45:12.345Z [test] [step: Run tests]       14 |   expect(linkElement).toBeInTheDocument();
2023-10-01T12:45:12.345Z [test] [step: Run tests]       15 | });

Why: The test failed because it attempted to access a property value of an undefined object. This usually happens when the component or its props are not correctly initialized in the test.

Suggested fix: Ensure that all necessary props and states are properly initialized in the test setup. Check the component rendering logic to ensure it handles undefined values gracefully.

3. [Linting failed]:[lint]

Source of Error:
2023-10-01T12:50:23.456Z [lint] [step: Run linter] /src/components/App.js
2023-10-01T12:50:23.456Z [lint] [step: Run linter]   10:5  error  'React' is defined but never used  no-unused-vars
2023-10-01T12:50:23.456Z [lint] [step: Run linter]   15:9  error  'useState' is assigned a value but never used  no-unused-vars
2023-10-01T12:50:23.456Z [lint] [step: Run linter] ✖ 2 problems (2 errors, 0 warnings)

Why: The linter found unused variables in the code. This is a common issue when variables are declared but not used anywhere in the code, which can lead to unnecessary clutter and potential confusion.

Suggested fix: Remove the unused variables React and useState from the code to pass the linting step. Ensure that all declared variables are used or necessary for the code logic.

AER Report: Operator UI CI ran successfully ✅

aer_workflow , commit

@cedric-cordenier cedric-cordenier force-pushed the CAPPL-332-fix-workflow-key branch from 8746cd0 to 735b4f2 Compare December 2, 2024 14:38
Comment on lines -472 to +474
if s.Config.Capabilities().WorkflowRegistry().Address() != "" {
err2 := app.GetKeyStore().Workflow().EnsureKey(rootCtx)
if err2 != nil {
return errors.Wrap(err2, "failed to ensure workflow key")
}
err2 := app.GetKeyStore().Workflow().EnsureKey(rootCtx)
if err2 != nil {
return errors.Wrap(err2, "failed to ensure workflow key")
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 This check was created because we said not all the node would have this key, wouldn't this assume the opposite?

Copy link
Contributor Author

@cedric-cordenier cedric-cordenier Dec 3, 2024

Choose a reason for hiding this comment

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

Yeah that's right! The problem is that in order to setup the WorkflowRegistry we need a key, and the check will only create one if there is a WorkflowRegistry set; the current workaround is to set it to a dummy address ("0x0") but that would still require two restarts

All in all it feels simpler and less error prone to just always create it; the cost of doing so should be very low.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok got it, thanks 👍🏼

@cedric-cordenier cedric-cordenier requested review from a team as code owners December 2, 2024 18:27
@cedric-cordenier cedric-cordenier added this pull request to the merge queue Dec 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 3, 2024
@cedric-cordenier cedric-cordenier added this pull request to the merge queue Dec 3, 2024
Merged via the queue into develop with commit 9deaaf5 Dec 3, 2024
157 of 160 checks passed
@cedric-cordenier cedric-cordenier deleted the CAPPL-332-fix-workflow-key branch December 3, 2024 12:54
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.

4 participants