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-14718 test: Fix reading of DAOS_TEST_INSECURE_MODE envar #13441

Merged
merged 4 commits into from
Jan 3, 2024
Merged

DAOS-14718 test: Fix reading of DAOS_TEST_INSECURE_MODE envar #13441

merged 4 commits into from
Jan 3, 2024

Conversation

tanabarr
Copy link
Contributor

@tanabarr tanabarr commented Dec 5, 2023 •
edited
Loading

The ConfigGenerateRun tests fail to use certs because insecure mode is
enabled when trying to access non-existent DAOS_INSECURE_MODE env.
Update env keyname to DAOS_TEST_INSECURE_MODE and explicitly set
allow_insecure mode to true in ConfigGenerateRun tests.

Test-tag: control,dmg_config_generate
Required-githooks: true

Signed-off-by: Tom Nabarro tom.nabarro@intel.com

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 watchers.
  • 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.

@tanabarr tanabarr requested a review from a team as a code owner December 5, 2023 10:57
@tanabarr tanabarr self-assigned this Dec 5, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2023 •
edited
Loading

Bug-tracker data:
Ticket title is 'ConfigGenerate tests failing due to certificate location issue'
Status is 'In Review'
Labels: 'ci_impact,weekly_test'
https://daosio.atlassian.net/browse/DAOS-14718

daosbuild1
daosbuild1 reviewed Dec 5, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

daltonbohning
daltonbohning reviewed Dec 5, 2023
View reviewed changes
Copy link
Contributor

@daltonbohning daltonbohning left a comment

Choose a reason for hiding this comment

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

Code LGTM, but since this touches a utility file it should run more tests. Thoughts @phender? Maybe just one more test that uses allow_insecure: True?
FWIW, this is a VM test that does: test_daos_control_config_basic

daltonbohning
daltonbohning previously approved these changes Dec 5, 2023
View reviewed changes
phender
phender previously approved these changes Dec 5, 2023
View reviewed changes
@phender
Copy link
Contributor

phender commented Dec 5, 2023

Code LGTM, but since this touches a utility file it should run more tests. Thoughts @phender? Maybe just one more test that uses allow_insecure: True? FWIW, this is a VM test that does: test_daos_control_config_basic

Since the removal of setting DAOS_INSECURE_MODE the control/config_generate_run.py is the only test that has failed. As this env was used to set the default value when the test neglected to set a value in its yaml file, I think the tag selection is sufficient.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13441/1/testReport/

@tanabarr
DAOS-14718 ftest: Fix reading of DAOS_TEST_INSECURE_MODE envar
a81022f 
The ConfigGenerateRun tests fail to use certs because insecure mode is
enabled when trying to access non-existent DAOS_INSECURE_MODE env.
Fix by updating env keyname to DAOS_TEST_INSECURE_MODE.

Test-tag: control,dmg_config_generate
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr tanabarr dismissed stale reviews from phender and daltonbohning via a81022f December 8, 2023 15:53
daosbuild1
daosbuild1 reviewed Dec 8, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

phender
phender previously approved these changes Dec 11, 2023
View reviewed changes
@tanabarr
Merge remote-tracking branch 'origin/master' into tanabarr/test-insec…
ae713d0 
…ure-mode

Test-tag: control,dmg_config_generate

Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
daosbuild1
daosbuild1 reviewed Dec 11, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-13441/3/testReport/

daosbuild1
daosbuild1 reviewed Dec 30, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Large completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-13441/4/execution/node/1281/log

@tanabarr
set allow_insecure explicitly
32e62f7 
Test-tag: control,dmg_config_generate

Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
daosbuild1
daosbuild1 reviewed Dec 31, 2023
View reviewed changes
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@ashleypittman
Copy link
Contributor

The linting issues in this PR are as a result of being behind master, this is something we're getting better at but the checks should run if you merge with master. The failures are because the checks aren't running properly so I can't say if they'll pass or not but looking at the PR they should.

@tanabarr tanabarr mentioned this pull request Jan 2, 2024
Merged
18 tasks
@tanabarr
Merge remote-tracking branch 'origin/master' into tanabarr/test-insec…
6e9fb76 
…ure-mode

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr
Copy link
Contributor Author

tanabarr commented Jan 2, 2024

Merged master and repushed with Features: control to both resolve linting issues fixed on master and check we can get a clean feature run for control.

@tanabarr tanabarr changed the title DAOS-14718 ftest: Fix reading of DAOS_TEST_INSECURE_MODE envar DAOS-14718 test: Fix reading of DAOS_TEST_INSECURE_MODE envar Jan 2, 2024
knard-intel
knard-intel approved these changes Jan 2, 2024
View reviewed changes
Copy link
Contributor

@knard-intel knard-intel left a comment

Choose a reason for hiding this comment

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

LGTM

@tanabarr tanabarr requested a review from a team January 3, 2024 00:14
@phender phender merged commit e0b7212 into master Jan 3, 2024
49 of 50 checks passed
@phender phender deleted the tanabarr/test-insecure-mode branch January 3, 2024 19:03
tanabarr added a commit that referenced this pull request Jan 3, 2024
@tanabarr
DAOS-14718 test: Fix reading of DAOS_TEST_INSECURE_MODE envar (#13441)
cf1713a 
The ConfigGenerateRun tests fail to use certs because insecure mode is
enabled when trying to access non-existent DAOS_INSECURE_MODE env.
Fix by updating env keyname to DAOS_TEST_INSECURE_MODE.

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
daltonbohning pushed a commit that referenced this pull request Jan 18, 2024
@tanabarr
DAOS-14718 test: Fix reading of DAOS_TEST_INSECURE_MODE envar (#13441) (
f13844d 
#13560)

The ConfigGenerateRun tests fail to use certs because insecure mode is
enabled when trying to access non-existent DAOS_INSECURE_MODE env.
Fix by updating env keyname to DAOS_TEST_INSECURE_MODE.

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daosbuild1 daosbuild1 daosbuild1 left review comments

@daltonbohning daltonbohning daltonbohning approved these changes

@knard-intel knard-intel knard-intel approved these changes

@phender phender Awaiting requested review from phender

Assignees

@tanabarr tanabarr

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tanabarr @phender @daosbuild1 @ashleypittman @daltonbohning @knard-intel