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.

Sign up for GitHub

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

Jump to bottom

DAOS-15915 cq: fix codespell errors #14439

Merged
merged 2 commits into from
May 24, 2024
Merged

DAOS-15915 cq: fix codespell errors #14439

merged 2 commits into from
May 24, 2024

Conversation

daltonbohning
Copy link
Contributor

Test-tag: pr,vm

That seem to be introduced by some implicit version change

Required-githooks: true

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate owners.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@daltonbohning daltonbohning self-assigned this May 24, 2024
@github-actions GitHub Actions
Copy link

Ticket title is 'fix new codespell errors'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-15915

@daltonbohning
DAOS-15915 cq: fix codespell errors
1c2432a 
Test-tag: pr,vm

That seem to be introduced by some implicit version change

Required-githooks: true

Signed-off-by: Dalton Bohning <dalton.bohning@intel.com>
@daltonbohning daltonbohning force-pushed the dbohning/daos-15915 branch from 2328907 to 1c2432a Compare May 24, 2024 15:29
@daosbuild1
Copy link
Collaborator

Test stage NLT on EL 8.8 completed with status FAILURE. https://build.hpdd.intel.com/job/daos-stack/job/daos/job/PR-14439/1/display/redirect

jolivier23
jolivier23 previously approved these changes May 24, 2024
View reviewed changes
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14439/2/testReport/

frostedcmos
frostedcmos previously approved these changes May 24, 2024
View reviewed changes
@daosbuild1
Copy link
Collaborator

Test stage Unit Test on EL 8.8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-14439/3/testReport/

@daltonbohning
Copy link
Contributor Author

The change to src/control/security/testdata/certs/source.txt
is causing the unit test to fail:
https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-14439/3/tests/

@kjacque Are you familiar with this? It looks like src/control/security/testdata/certs/source.txt is hashed and expected to match src/control/security/testdata/certs/RSA.golden

Is it simple enough to regenerate a golden hash?

@daltonbohning
Copy link
Contributor Author

The change to src/control/security/testdata/certs/source.txt is causing the unit test to fail: https://build.hpdd.intel.com/blue/organizations/jenkins/daos-stack%2Fdaos/detail/PR-14439/3/tests/

@kjacque Are you familiar with this? It looks like src/control/security/testdata/certs/source.txt is hashed and expected to match src/control/security/testdata/certs/RSA.golden

Is it simple enough to regenerate a golden hash?

A workaround is to tell codespell to ignore that file

@daltonbohning
ignore certs/source.txt
db3e563 
Test-tag: pr,vm

Required-githooks: true

Signed-off-by: Dalton Bohning <dalton.bohning@intel.com>
@daltonbohning daltonbohning dismissed stale reviews from frostedcmos and jolivier23 via db3e563 May 24, 2024 19:01
phender
phender reviewed May 24, 2024
View reviewed changes
ci/codespell.ignores
@@ -36,3 +36,6 @@ laf
cacl
chk
falloc
rin
assertIn
checkin
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be added? It seems like a legitimate spelling mistake - where either 'checking' or 'check in' should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's terminology in our code. E.g.

daos/src/tests/ftest/cart/test_rpc_to_ghost_rank.c

Line 97 in 1268dc4

printf("test_group server recv'd checkin, opc: %#x.\n",

@daltonbohning daltonbohning marked this pull request as ready for review May 24, 2024 22:07
@daltonbohning daltonbohning requested review from a team as code owners May 24, 2024 22:07
daltonbohning
daltonbohning commented May 24, 2024
View reviewed changes
pyproject.toml
@@ -5,7 +5,7 @@ line_length = 99
skip_gitignore = true

[tool.codespell]
skip = './src/control/vendor,./src/control/go.sum,./.git,./src/rdb/raft,./build,./install,./venv,./src/control/vendor/'
skip = './src/control/vendor,./src/control/go.sum,./.git,./src/rdb/raft,./build,./install,./venv,./src/control/vendor/,./src/control/security/testdata/certs/source.txt'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really a workaround since I'm not sure how to regenerate the .golden file. I'll try to follow up with a proper fix but this gets the check passing in master again

@daltonbohning daltonbohning requested a review from a team May 24, 2024 22:13
@daltonbohning
Copy link
Contributor Author

Skipped Jenkins HW testing since changes are only in code comments and GHA config

@daltonbohning daltonbohning added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label May 24, 2024
@daltonbohning daltonbohning merged commit 3a1d29c into master May 24, 2024
69 checks passed
@daltonbohning daltonbohning deleted the dbohning/daos-15915 branch May 24, 2024 22:15
daltonbohning added a commit that referenced this pull request May 28, 2024
@daltonbohning
DAOS-15915 cq: fix codespell errors (#14439)
3fe15c1 
Test-tag: pr,vm

Fix codespell errors that seem to be introduced by some implicit version change.
Also temporarily skip checking ./src/control/security/testdata/certs/source.txt
until a new .golden is generated.

Required-githooks: true

Signed-off-by: Dalton Bohning <dalton.bohning@intel.com>
daltonbohning added a commit that referenced this pull request May 29, 2024
@daltonbohning
DAOS-15915 cq: fix codespell errors (#14439) (#14459)
06711d7 
Fix codespell errors that seem to be introduced by some implicit version change.
Also temporarily skip checking ./src/control/security/testdata/certs/source.txt
until a new .golden is generated.

Signed-off-by: Dalton Bohning <dalton.bohning@intel.com>
@mjmac mjmac mentioned this pull request Nov 13, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frostedcmos frostedcmos frostedcmos approved these changes

@ryon-jensen ryon-jensen ryon-jensen approved these changes

@phender phender Awaiting requested review from phender

@jolivier23 jolivier23 Awaiting requested review from jolivier23

Assignees

@daltonbohning daltonbohning

Labels
forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@daltonbohning @daosbuild1 @jolivier23 @ryon-jensen @frostedcmos @phender