Skip to content

Rename NodeScanV2 to NodeInventory#3755

Merged
vikin91 merged 14 commits intomasterfrom
pr/rename-nodeScanV2
Nov 30, 2022
Merged

Rename NodeScanV2 to NodeInventory#3755
vikin91 merged 14 commits intomasterfrom
pr/rename-nodeScanV2

Conversation

@vikin91
Copy link
Contributor

@vikin91 vikin91 commented Nov 9, 2022 •
edited
Loading

Description

Scope: This PR renames storage.NodeScanV2 to storage.NodeInventory.

Why?
This is done to avoid confusion between the word scan being understood as:

  1. A result returned by the Scanner (containing list of vulnerabilities)
  2. A result of Compliance searching files on a disk.

We hereby propose to call the (1) a scan, whereas the (2) a node inventory.

Checklist

  • Investigated and inspected CI test results

Do not apply

  • Unit test and regression tests added
  • Evaluated and added CHANGELOG entry if required
  • Determined and documented upgrade steps
  • Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

Reason: This is a pure rename refactor

Testing Performed

  • CI
  • Manual tests consisting of starting a local cluster and looking in the
    logs of Sensor, Central, and Compliance

@openshift-ci
Copy link

openshift-ci bot commented Nov 9, 2022

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@vikin91 vikin91 mentioned this pull request Nov 9, 2022
Merged
2 tasks
@vikin91
Copy link
Contributor Author

vikin91 commented Nov 9, 2022 •
edited
Loading

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@vikin91
Copy link
Contributor Author

vikin91 commented Nov 9, 2022

/test all

@openshift-ci
Copy link

openshift-ci bot commented Nov 9, 2022

@vikin91: No presubmit jobs available for stackrox/stackrox@pr/ROX-12835-sensor-nodescan-v2

Details

In response to this:

/test all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@vikin91 vikin91 mentioned this pull request Nov 9, 2022
Closed
8 tasks
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from 1c6b5a7 to 7ffbcf8 Compare November 9, 2022 14:32
Base automatically changed from pr/ROX-12835-sensor-nodescan-v2 to master November 11, 2022 08:39
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from 7ffbcf8 to af46a2d Compare November 11, 2022 08:45
@vikin91 vikin91 changed the base branch from master to pr/ROX-12835-followup-1 November 15, 2022 08:52
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from af46a2d to ce78183 Compare November 15, 2022 08:52
@vikin91 vikin91 mentioned this pull request Nov 15, 2022
Merged
9 tasks
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from ce78183 to 681ff0b Compare November 15, 2022 13:08
@vikin91 vikin91 marked this pull request as ready for review November 15, 2022 16:58
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from 681ff0b to 66d340b Compare November 16, 2022 15:40
@vikin91 vikin91 force-pushed the pr/ROX-12835-followup-1 branch 2 times, most recently from d7353ae to ee4d20d Compare November 21, 2022 10:52
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from 66d340b to b5caa72 Compare November 21, 2022 10:55
@vikin91 vikin91 mentioned this pull request Nov 22, 2022
Merged
4 tasks
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from b5caa72 to 9e75bcd Compare November 23, 2022 09:12
@vikin91 vikin91 mentioned this pull request Nov 23, 2022
Closed
5 tasks
@vikin91 vikin91 force-pushed the pr/rename-nodeScanV2 branch from 9e75bcd to 9414f33 Compare November 23, 2022 14:29
vikin91
vikin91 commented Nov 23, 2022
View reviewed changes
proto/storage/node.proto
}

message NodeScanV2 {
string node_id = 1 [(gogoproto.moretags) = 'search:"Node ID,store" sql:"pk,type(uuid)"'];
Copy link
Contributor Author

@vikin91 vikin91 Nov 23, 2022

Choose a reason for hiding this comment

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

TODO: Readd the ,type(uuid)

@vikin91 vikin91 force-pushed the pr/ROX-12835-followup-1 branch from 56dab50 to e385c20 Compare November 28, 2022 17:15
Base automatically changed from pr/ROX-12835-followup-1 to master November 29, 2022 10:10
@vikin91
Copy link
Contributor Author

vikin91 commented Nov 29, 2022

/test all

@ghost
Copy link

ghost commented Nov 29, 2022 •
edited by ghost
Loading

Images are ready for the commit at f8b397f.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.73.x-106-gf8b397fae8.

@vikin91
Copy link
Contributor Author

vikin91 commented Nov 29, 2022

/test all

@vikin91 vikin91 marked this pull request as ready for review November 29, 2022 14:18
msugakov
msugakov reviewed Nov 29, 2022
View reviewed changes
Copy link
Contributor

@msugakov msugakov left a comment

Choose a reason for hiding this comment

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

I came here to write this comment.

msugakov
msugakov reviewed Nov 29, 2022
View reviewed changes
This was referenced Nov 29, 2022
Closed
Closed
@vikin91
Copy link
Contributor Author

vikin91 commented Nov 29, 2022

Thanks @msugakov !
I did another pass with more permissive regex and caught one more place mentioning nodescan. I hope now it looks much better.

@vikin91
Copy link
Contributor Author

vikin91 commented Nov 29, 2022

/test all

msugakov
msugakov reviewed Nov 29, 2022
View reviewed changes
sensor/common/compliance/node_inventory_handler_test.go Outdated
var _ suite.TearDownTestSuite = (*NodeInventoryTestSuite)(nil)

type NodeScanHandlerTestSuite struct {
type NodeInventoryTestSuite struct {
Copy link
Contributor

@msugakov msugakov Nov 29, 2022

Choose a reason for hiding this comment

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

Suggested change
type NodeInventoryTestSuite struct {
type NodeInventoryHandlerTestSuite struct {

Because we are testing Handler

jschnath
jschnath approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@jschnath jschnath left a comment

Choose a reason for hiding this comment

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

LGTM and I found 0 instances of NodeScanV2 left in the code

Maddosaurus
Maddosaurus reviewed Nov 30, 2022
View reviewed changes
Copy link
Contributor

@Maddosaurus Maddosaurus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(posted a comment instead of an approval)

Maddosaurus
Maddosaurus approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@Maddosaurus Maddosaurus left a comment

Choose a reason for hiding this comment

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

LGTM, let's 🚀

@vikin91 vikin91 merged commit 0ac27d7 into master Nov 30, 2022
@vikin91 vikin91 deleted the pr/rename-nodeScanV2 branch November 30, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msugakov msugakov msugakov approved these changes

@jschnath jschnath jschnath approved these changes

+1 more reviewer

@Maddosaurus Maddosaurus Maddosaurus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/central area/sensor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vikin91 @msugakov @Maddosaurus @jschnath