-
Notifications
You must be signed in to change notification settings - Fork 114
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
include tags on nodemanager process node #253
Conversation
this change is in support of the work towards the one node view. also included is a change to ensure we only add a tag if it does not already exist in the db and only add a node id-tag id association if it does not already exist Signed-off-by: Victoria Jeffrey <[email protected]>
node.GetName(), node.GetPlatformName(), node.GetPlatformRelease(), | ||
nodeState, lastContact, node.GetSourceId(), node.GetSourceRegion(), | ||
node.GetSourceAccountId(), node.GetJobUuid()) | ||
err = Transact(db, func(tx *DBTrans) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the Transact here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -67,6 +68,7 @@ type InspecJob struct { | |||
ProfilesOwner string `json:"profiles_owner,omitempty"` | |||
InternalProfiles []string `json:"internal_profiles,omitempty"` | |||
MachineIdentifier string `json:"machine_identifier,omitempty"` | |||
Tags []*common.Kv `json:"kv,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be called tags
instead?
`json:"tags,omitempty"`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh ya it totally should!
} | ||
links = append(links, &link) | ||
} | ||
return trans.Insert(links...) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this is asking for quite a bit of rework, but I feel like this database syncing stuff would be better done in the database, by making the relevant columns unique and doing INSERT ... ON CONFLICT DO NOTHING
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point, that would be cleaner. i'll give that a try.
The CI failure is probably not related to this PR and I'm guessing just needs a retry, but I've never seen this one before. |
I think the CI failure is b/c i'm branched off a branch |
that's so annoying. i went to change the base branch and it closed my pr and then told me i can't change the base branch on a closed pr :/ |
well anyways, ill work on the improvements mentioned and the reopen with a new pull request since it won't allow me to re-open this one |
🔩 Description
this change is in support of the work towards the
one node view. also included is a change to ensure we only
add a tag if it does not already exist in the db and only
add a node id-tag id association if it does not already exist
👍 Definition of Done
tags are passed through from scan job -> report
tags are added as part of the processNode codepath
tags are not added to the db if they already exist in the system
node tags associations are not added to the db if they already exist in the system
⛓️ Related Resources
#192