Skip to content

Rename NodeScanV2 to NodeInventory - #3755

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

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

Conversation

@vikin91

@vikin91 vikin91 commented Nov 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

openshift-ci Bot commented Nov 9, 2022

Copy link
Copy Markdown

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

vikin91 commented Nov 9, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@vikin91

vikin91 commented Nov 9, 2022

Copy link
Copy Markdown
Contributor Author

/test all

@openshift-ci

openshift-ci Bot commented Nov 9, 2022

Copy link
Copy Markdown

@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
Comment thread proto/storage/node.proto
}

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

@vikin91 vikin91 Nov 23, 2022

Copy link
Copy Markdown
Contributor Author

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

vikin91 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

/test all

@ghost

ghost commented Nov 29, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

Images are ready for the commit at f8b397f.

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

@vikin91

vikin91 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

/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

@msugakov msugakov left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread sensor/common/compliance/nodescan_handler.go
msugakov
msugakov reviewed Nov 29, 2022
View reviewed changes
Comment thread sensor/common/compliance/nodescan_handler_impl.go Outdated
Comment thread central/sensor/service/pipeline/nodescansv2/pipeline.go Outdated
Comment thread central/sensor/service/pipeline/nodescansv2/pipeline.go Outdated
Comment thread compliance/collection/main.go
Comment thread sensor/common/compliance/nodescan_handler_impl.go Outdated
This was referenced Nov 29, 2022
Closed
Closed
@vikin91

vikin91 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

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

vikin91 commented Nov 29, 2022

Copy link
Copy Markdown
Contributor Author

/test all

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

type NodeScanHandlerTestSuite struct {
type NodeInventoryTestSuite struct {

@msugakov msugakov Nov 29, 2022

Copy link
Copy Markdown
Contributor

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

@jschnath jschnath left a comment

Copy link
Copy Markdown
Contributor

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

@Maddosaurus Maddosaurus left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

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

@Maddosaurus Maddosaurus left a comment

Copy link
Copy Markdown
Contributor

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