Skip to content

Conversation

javuto
Copy link
Collaborator

@javuto javuto commented Oct 15, 2025

Implementation of #736 to skip automatic tag creation for UUIDs, localnames and hostnames.

@javuto javuto requested a review from Copilot October 15, 2025 17:05
@javuto javuto added osctrl-tls osctrl-tls related changes 🏷️ tags Tags related issues labels Oct 15, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes automatic tag creation for UUIDs, localnames, and hostnames, and surfaces a count of tagged nodes in the admin UI.

  • Stop generating/handling tag types: uuid, localname, hostname (decorators, parsers, custom setters, CLI options, tests).
  • Adjust auto-tagging to only use environment and platform.
  • Add tagged-nodes count to the Tags page, including a new TagCounter type and CountTaggedNodes method.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/tags/utils_test.go Updates tests to align with removed tag types (uuid, localname).
pkg/tags/utils.go Removes constants and switch cases for uuid/localname/hostname across helpers.
pkg/tags/tags.go Adds TagCounter and CountTaggedNodes; fixes GetTaggedNodes query; limits AutoTagNode to env+platform.
cmd/cli/node.go Removes CLI tag-type options for uuid, localname, hostname.
cmd/admin/templates/tags.html Adds “Tagged Nodes” column; removes uuid/localname/hostname tag type options.
cmd/admin/handlers/types-templates.go Adds CounterTags to template data.
cmd/admin/handlers/templates.go Wires CountTaggedNodes into TagsGETHandler.

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +543 to +548
for _, t := range tags {
tagged, err := m.GetTaggedNodes(t)
if err != nil {
return tagCounter, err
}
tagCounter[t.ID] = len(tagged)
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

This implementation causes an N+1 query pattern by issuing one query per tag and loading full rows just to count. Replace with a single grouped COUNT query, e.g., select admin_tag_id, count(*) grouped by admin_tag_id, to populate TagCounter in one round-trip.

Suggested change
for _, t := range tags {
tagged, err := m.GetTaggedNodes(t)
if err != nil {
return tagCounter, err
}
tagCounter[t.ID] = len(tagged)
if len(tags) == 0 {
return tagCounter, nil
}
// Collect tag IDs
tagIDs := make([]uint, len(tags))
for i, t := range tags {
tagIDs[i] = t.ID
}
// Result struct for query
type result struct {
AdminTagID uint
Count int
}
var results []result
// Grouped count query
if err := m.DB.
Model(&TaggedNode{}).
Select("admin_tag_id, COUNT(*) as count").
Where("admin_tag_id IN ?", tagIDs).
Group("admin_tag_id").
Scan(&results).Error; err != nil {
return tagCounter, err
}
// Populate TagCounter from results
for _, r := range results {
tagCounter[r.AdminTagID] = r.Count
}
// Ensure all tags are present in the map (with zero if not found)
for _, id := range tagIDs {
if _, ok := tagCounter[id]; !ok {
tagCounter[id] = 0
}

Copilot uses AI. Check for mistakes.

Comment on lines 143 to 147
<option value="0">Environment</option>
<option value="1">UUID</option>
<option value="2">Platform</option>
<option value="3">Localname</option>
<option value="4">Custom</option>
<option value="5">Unknown</option>
<option value="6">Tag</option>
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

These magic numbers (0, 2, 4, 5, 6) must stay in sync with TagType constants. To avoid drift, consider exposing the numeric constants to the template data (e.g., map of name->value) and render options from that source.

Copilot uses AI. Check for mistakes.

Copy link
Contributor

@zhuoyuan-liu zhuoyuan-liu left a comment

Choose a reason for hiding this comment

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

Great change! I think the documentation for tag cleanup is the only missing part.

ActionUntag string = "untag"
// TagCustomEnv as custom tag for environment name
TagCustomEnv string = TagTypeEnvStr
// TagCustomUUID as custom tag for node UUID
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we remove these consnt as well?

// TagTypeEnv as tag type for environment name
	TagTypeEnv uint = 0
	// TagTypeUUID as tag type for node UUID
	TagTypeUUID uint = 1
	// TagTypePlatform as tag type for node platform
	TagTypePlatform uint = 2
	// TagTypeLocalname as tag type for node localname
	TagTypeLocalname uint = 3
	// TagTypeCustom as tag type for custom tags
	TagTypeCustom uint = 4
	// TagTypeUnknown as tag type for unknown tags
	TagTypeUnknown uint = 5
	// TagTypeTag as tag type for regular tags
	TagTypeTag uint = 6
	// TagTypeHostname as tag type for hostname tags
	TagTypeHostname uint = 7

}

// CountTaggedNodes to count tagged nodes for a given list of tags
func (m *TagManager) CountTaggedNodes(tags []AdminTag) (TagCounter, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have the same suggestion as Copilot: we should change it to use a single aggregate query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

osctrl-tls osctrl-tls related changes 🏷️ tags Tags related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants