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.

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

nodemanager nodes: fix platform and tags info, add state to possible sort options #3596

Merged
merged 10 commits into from
May 7, 2020

Conversation

vjeffrey
Copy link

@vjeffrey vjeffrey commented May 5, 2020

🔩 Description: What code changed, and why?

a few issues:

  • platform version was showing up twice in the nodemanager nodes: this is bc we were mistakenly sending the full platform field + the version, instead of either just sending the full field or jsut sending the family and version. i switched to used family for now, for consistency with the compliance side of things, but we may want to come back and just have everything send the full field? not sure yet. should discuss how customers would want to filter that data.
  • tags: added environment value as a tag to the node, and also modified the compliance bits to include chef tags in the tags information sent to nodemanager
  • state: we did not previously allow sorting by state; we should!

⛓️ Related Resources

#3515

👍 Definition of Done

platform information doesn't double up. tags are all sent over, including env. can filter by state.

👟 How to Build and Test the Change

go_update_component compliance-service
same for ingest-service
and nodemanager-service
send in some reports, e.g. load_compliance_reports
check the data returned by api/v0/nodes, accessible at automate-url/nodes in the ui

✅ Checklist

Aha! Link: https://chef.aha.io/features/SH-1780

@@ -166,6 +166,7 @@ var nodesSortFields = map[string]string{
"status": "LOWER(n.status)",
"manager": "LOWER(n.manager)",
"last_contact": "n.last_contact",
"state": "n.source_state",
Copy link
Author

Choose a reason for hiding this comment

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

leaving as draft til i get a test written for this

Copy link
Author

Choose a reason for hiding this comment

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

done!

@vjeffrey vjeffrey requested a review from a team May 6, 2020 04:56
@vjeffrey vjeffrey marked this pull request as ready for review May 6, 2020 18:37

return &manager.NodeMetadata{
Uuid: node.EntityUuid,
Name: node.NodeName,
PlatformName: node.Platform,
PlatformName: node.PlatformFamily,
Copy link
Author

@vjeffrey vjeffrey May 6, 2020

Choose a reason for hiding this comment

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

maybe we want to eventually go to just sending the platform full field? for now, i switched it to using family for consistency with compliance side of things, since it's sending name/version as two sep fields, and two sep fields is what nodemanager expects

Copy link
Contributor

@lancewf lancewf May 6, 2020

Choose a reason for hiding this comment

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

For our example CCR

The platform_family "rhel" is different than the platform "redhat". I don't know if this will be a problem.

For this other example

the platform_family is "debian" but the platform is "ubuntu". That might be confusing to show "debian 18.04" for the platform when debian is only on version 10.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I have a solution 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is one solution #3612

@vjeffrey
Copy link
Author

vjeffrey commented May 6, 2020

ill get this rebased

Victoria Jeffrey added 8 commits May 6, 2020 16:59
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Signed-off-by: Victoria Jeffrey <[email protected]>
Copy link
Contributor

@lancewf lancewf left a comment

Choose a reason for hiding this comment

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

Works great and looks good!

@vjeffrey vjeffrey force-pushed the vj/platform-tags-state branch from 154753b to 0cd68bf Compare May 6, 2020 23:03
@@ -355,6 +355,10 @@ func (ccr *ChefClientRun) CloudProvider() string {
return EmptyStringIfNil(cloud["provider"])
}

func (ccr *ChefClientRun) Platform() string {
Copy link
Author

Choose a reason for hiding this comment

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

oh i think this is from the rebase and no longer needed?

Copy link
Author

Choose a reason for hiding this comment

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

oh nm we call it below

Signed-off-by: Victoria Jeffrey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants