RHCLOUD-41972: Fix quickstart content structure to store complete data#367
Open
catastrophe-brandon wants to merge 6 commits intomainfrom
Open
RHCLOUD-41972: Fix quickstart content structure to store complete data#367catastrophe-brandon wants to merge 6 commits intomainfrom
catastrophe-brandon wants to merge 6 commits intomainfrom
Conversation
Contributor
Reviewer's GuideThis PR revamps how quickstart content and tags are loaded and stored by introducing a comprehensive content wrapper (including apiVersion, kind, spec and metadata), refactors the database seeding into modular phases with helper classes (FileHelper and DBHelper) for consistent error handling and logging, and updates tests to verify that full quickstart data is preserved. Sequence diagram for modular database seeding phases using helperssequenceDiagram
participant M as "main()"
participant DB as "database.SeedTags()"
participant FH as "FileHelper"
participant DH as "DBHelper"
M->>DB: Call SeedTags()
DB->>DB: clearOldContent()
DB->>DH: Use DBHelper for batch deletes
DB->>DB: seedDefaultTags()
DB->>DH: Use DBHelper for FindOrCreate
DB->>DB: findTags()
DB->>FH: Use FileHelper for file discovery
DB->>DB: For each MetadataTemplate
DB->>DB: processQuickstartPhase/processHelpTopicPhase
DB->>DH: Use DBHelper for DB operations
DB->>FH: Use FileHelper for content parsing
DB->>DB: SeedFavorites()
DB->>DH: Use DBHelper for batch create
DB-->>M: Return seeding result
Class diagram for SeedingResult structclassDiagram
class SeedingResult {
+QuickstartsProcessed : int
+QuickstartsCreated : int
+QuickstartsUpdated : int
+HelpTopicsProcessed : int
+HelpTopicsCreated : int
+HelpTopicsUpdated : int
+TagsCreated : int
+FavoritesRestored : int
+Errors : []error
+addError(msg string) error
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider changing the APIVersion and Kind fields from interface{} to string for better type safety and clarity in the wrapper struct.
- In content_verification_test, add assertions for actual spec values (e.g. displayName, description) and the expected tags rather than only checking for non-nil fields to make the test more robust.
- Use an embedded or dedicated test fixture for the sample quickstart YAML instead of a relative file path to avoid CI path-related flakiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider changing the APIVersion and Kind fields from interface{} to string for better type safety and clarity in the wrapper struct.
- In content_verification_test, add assertions for actual spec values (e.g. displayName, description) and the expected tags rather than only checking for non-nil fields to make the test more robust.
- Use an embedded or dedicated test fixture for the sample quickstart YAML instead of a relative file path to avoid CI path-related flakiness.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
catastrophe-brandon
force-pushed
the
btweed/fix/quickstarts
branch
from
e81e8c7 to
be2dbdb
Compare
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Much of the tag‐processing code still uses raw DB calls—consider switching those to your new DBHelper methods so error handling and logging stay consistent across the board.
- The function named findTags now actually discovers metadata templates for quickstarts and topics; renaming it to something like discoverMetadata or loadMetadataTemplates will make its purpose clearer.
- There are a lot of Infof/Debugf calls sprinkled throughout the seeding and helper logic—review your log levels to avoid excessive noise in production builds and consider demoting some logs to Debug only.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Much of the tag‐processing code still uses raw DB calls—consider switching those to your new DBHelper methods so error handling and logging stay consistent across the board.
- The function named findTags now actually discovers metadata templates for quickstarts and topics; renaming it to something like discoverMetadata or loadMetadataTemplates will make its purpose clearer.
- There are a lot of Infof/Debugf calls sprinkled throughout the seeding and helper logic—review your log levels to avoid excessive noise in production builds and consider demoting some logs to Debug only.
## Individual Comments
### Comment 1
<location> `pkg/database/file_utils.go:89` </location>
<code_context>
+
+// AddTagsToContent reads a content file, adds tags to its metadata, and returns JSON
+// Handles various file structures with fallback logic for robustness
+func (h *FileHelper) AddTagsToContent(path string, tags interface{}) ([]byte, error) {
+ h.logger.Debugf("Adding tags to content: %s", path)
+ raw, err := os.ReadFile(path)
</code_context>
<issue_to_address>
AddTagsToContent assumes the file structure matches contentWrapper.
Files with structures that differ from contentWrapper may not receive tags as intended. Please add validation or fallback logic to handle unexpected file formats.
Suggested implementation:
```golang
var wrap contentWrapper
if err := yaml.Unmarshal(raw, &wrap); err != nil {
// Fallback: try to unmarshal into a generic map
var generic map[string]interface{}
if err2 := yaml.Unmarshal(raw, &generic); err2 != nil {
return nil, fmt.Errorf("unmarshal %s: %w", path, err)
}
// Add tags to a top-level "metadata" key
if generic["metadata"] == nil {
generic["metadata"] = make(map[string]interface{})
}
metadata, ok := generic["metadata"].(map[string]interface{})
if !ok {
// If metadata is not a map, replace it
metadata = make(map[string]interface{})
generic["metadata"] = metadata
}
metadata["tags"] = tags
return json.Marshal(generic)
}
if wrap.Metadata == nil {
wrap.Metadata = make(map[string]interface{})
```
```golang
wrap.Metadata["tags"] = tags
return json.Marshal(wrap)
```
</issue_to_address>
### Comment 2
<location> `pkg/database/db_utils.go:125` </location>
<code_context>
+
+// ProcessBatch processes a batch of items with error collection and progress logging
+// Returns (successCount, errors) - callers can aggregate errors if needed
+func (h *DBHelper) ProcessBatch(items []interface{}, processor func(interface{}) error, batchName string) (successCount int, errors []error) {
+ h.logger.Infof("Starting batch %s (%d items)", batchName, len(items))
+
</code_context>
<issue_to_address>
ProcessBatch does not support early termination on critical errors.
Allow the processor to signal early termination on critical errors, or clarify in documentation that all items are processed regardless of errors.
Suggested implementation:
```golang
import (
"fmt"
"strings"
"errors"
"gorm.io/gorm"
"github.com/sirupsen/logrus"
)
// ErrCriticalBatchError can be returned by processor to signal early termination of batch processing.
var ErrCriticalBatchError = errors.New("critical batch error: early termination requested")
// ProcessBatch processes a batch of items with error collection and progress logging.
// If the processor returns ErrCriticalBatchError, processing stops early.
// Returns (successCount, errors) - callers can aggregate errors if needed.
func (h *DBHelper) ProcessBatch(items []interface{}, processor func(interface{}) error, batchName string) (successCount int, errors []error) {
h.logger.Infof("Starting batch %s (%d items)", batchName, len(items))
```
```golang
for i, item := range items {
err := processor(item)
if err != nil {
errors = append(errors, fmt.Errorf("item %d: %w", i, err))
if errors.Is(err, ErrCriticalBatchError) {
h.logger.Errorf("Critical error encountered in batch %s at item %d: %v. Terminating batch early.", batchName, i, err)
break
}
} else {
successCount++
}
}
h.logger.Infof("Finished batch %s: %d succeeded, %d errors", batchName, successCount, len(errors))
return successCount, errors
```
</issue_to_address>
### Comment 3
<location> `pkg/database/db_seed.go:494` </location>
<code_context>
- DB.Create(&newTag)
- originalTag = newTag
- }
+// processQuickstartTags handles tag processing for a quickstart
+func processQuickstartTags(template MetadataTemplate, quickstart models.Quickstart, result *SeedingResult) error {
+ for _, tag := range template.Tags {
</code_context>
<issue_to_address>
Consider refactoring the duplicated tag-processing logic into a single helper method on DBHelper to simplify both quickstart and help topic phases.
Here’s one way to collapse most of that duplicated looping and error-handling in your two “process…Tags” helpers into a single, small helper on your `DBHelper`. Once you have this, both quickstarts and helptopics become a one-liner:
```go
// in your dbhelper.go
func (h *DBHelper) ProcessTagsFor(
item interface{}, // &quickstart or &helpTopic
assocName string, // "Quickstarts" or "HelpTopics"
tagTemplates []TagTemplate,
) (created int, errs []error) {
for _, tt := range tagTemplates {
tag := models.Tag{Type: models.TagType(tt.Kind), Value: tt.Value}
// lookup or create
if _, err := h.FindOrCreate(
&tag,
map[string]interface{}{"type": tag.Type, "value": tag.Value},
"tag", tt.Value,
); err != nil {
errs = append(errs, err)
continue
}
// append association
if err := h.AppendAssociation(&tag, assocName, item, "tag-"+assocName, tt.Value); err != nil {
errs = append(errs, err)
continue
}
created++
}
return
}
```
Then your phases shrink to:
```go
func processQuickstartPhase(t MetadataTemplate, defs map[string]models.Tag, res *SeedingResult) {
qs, isNew, err := seedQuickstart(t, defs["quickstart"])
if err != nil { /*…*/ }
res.QuickstartsProcessed++
if isNew { res.QuickstartsCreated++ } else { res.QuickstartsUpdated++ }
// clear & re-tag in one call
DB.Model(&qs).Association("Tags").Clear()
if _, errs := dbHelper.ProcessTagsFor(&qs, "Quickstarts", t.Tags); len(errs) > 0 {
res.Errors = append(res.Errors, errs...)
}
}
```
```go
func processHelpTopicPhase(t MetadataTemplate, defs map[string]models.Tag, res *SeedingResult) {
hts, created, err := seedHelpTopic(t, defs["helptopic"])
if err != nil { /*…*/ }
res.HelpTopicsProcessed += len(hts)
res.HelpTopicsCreated += created
res.HelpTopicsUpdated += len(hts) - created
// for each topic just reuse the same helper
for i := range hts {
DB.Model(&hts[i]).Association("Tags").Clear()
if _, errs := dbHelper.ProcessTagsFor(&hts[i], "HelpTopics", t.Tags); len(errs) > 0 {
res.Errors = append(res.Errors, errs...)
}
}
}
```
Benefits:
- All of your lookup/create/associate/save boilerplate moves into one small function.
- Error-collection and counters happen in a single place.
- You’ve eliminated two large, near-identical loops (`processQuickstartTags` vs `processHelpTopicTags`) plus half a dozen logging lines.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@sourcery-ai resolve |
catastrophe-brandon
changed the title
catastrophe-brandon
force-pushed
the
btweed/fix/quickstarts
branch
from
be2dbdb to
fee0dc8
Compare
- Update contentWrapper struct to match OpenShift QuickStart format - Add apiVersion, kind, and spec fields to preserve full content - Previously only metadata was stored, losing actual quickstart content - Add verification test to ensure complete content storage - Fixes issue where spec section (displayName, description, etc.) was lost 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
catastrophe-brandon
force-pushed
the
btweed/fix/quickstarts
branch
from
fee0dc8 to
64f86ff
Compare
Contributor
justinorringer
left a comment
justinorringer
left a comment
There was a problem hiding this comment.
Tiny change to the log, otherwise I think it's worth a try!
| panic(err) | ||
| logrus.Fatalf("Database migration failed: %v", err) | ||
| } | ||
| logrus.Info("Database migration completed successfully") |
Contributor
There was a problem hiding this comment.
Shouldn't there be an else here? It'll print both failed and success haha.
| if err := database.SeedTags(); err != nil { | ||
| logrus.Fatalf("Database seeding failed: %v", err) | ||
| } | ||
| logrus.Info("Database seeding completed successfully") |
Contributor
There was a problem hiding this comment.
I'll also add we need to still panic/exit the process with non-zero if any errors
| if err := DB.Save(&defaultTag).Error; err != nil { | ||
| return returnValue, createdCount, fmt.Errorf("failed to save default tag for updated help topic %s: %w", nameStr, err) | ||
| } | ||
| DB.Save(&defaultTag) |
Contributor
There was a problem hiding this comment.
Capturing the errors for these makes sense :)
Contributor
There was a problem hiding this comment.
@justinorringer can you elaborate? Do you mean we should defer the return until we try saving all the entities?
florkbr
requested changes
Oct 1, 2025
|
|
||
| ### Build and Deployment | ||
| - Generate OpenAPI spec: `make generate-spec` | ||
| - Build Docker image: `docker build . -t quickstarts:latest` |
Contributor
There was a problem hiding this comment.
We should probably update this later to try with podman (not blocking)
| if err := database.SeedTags(); err != nil { | ||
| logrus.Fatalf("Database seeding failed: %v", err) | ||
| } | ||
| logrus.Info("Database seeding completed successfully") |
Contributor
There was a problem hiding this comment.
I'll also add we need to still panic/exit the process with non-zero if any errors
| fileHelper := NewFileHelper("test") | ||
|
|
||
| // Test with the hcs-getting-started quickstart | ||
| contentPath := "../docs/quickstarts/hcs-getting-started/hcs-getting-started.yml" |
Contributor
There was a problem hiding this comment.
We may want a dedicated test yaml in case someone removes this in the future, but for now 👍
| assert.NotNil(t, parsed["spec"], "Should have spec") | ||
| } | ||
|
|
||
| func getKeys(m map[string]interface{}) []string { |
Contributor
There was a problem hiding this comment.
We can probably reuse logic from another package for this, but for now 👍
|
|
||
| err = yaml.Unmarshal(yamlfile, &template) | ||
| if err != nil { | ||
| t.Logf("Warning: Failed to unmarshal metadata file %s: %v", file, err) |
Contributor
There was a problem hiding this comment.
It would be great to run this as part of the Konflux pipeline to validate PRs. I would also expect us to throw an error if one of the yamls is malformed preventing the PR from being merged.
Contributor
There was a problem hiding this comment.
It would also be awesome to add strict enforcement to the structure of the docs directory. So if I try to add a quickstart in the wrong place it blows up. Future JIRA link needed.
| logrus.Debugf("Processing tag: %s=%s for quickstart %s", tag.Kind, tag.Value, quickstart.Name) | ||
|
|
||
| quickstart.Tags = append(quickstart.Tags, originalTag) | ||
| r := DB.Preload("Quickstarts").Where("type = ? AND value = ?", models.TagType(newTag.Type), newTag.Value).Find(&originalTag) |
Contributor
There was a problem hiding this comment.
Should we use the dbHelper?
|
|
||
| // Create tag-quickstart association | ||
| if err := DB.Model(&originalTag).Association("Quickstarts").Append(&quickstart); err != nil { | ||
| errMsg := fmt.Sprintf("failed to create tag association %s=%s for quickstart %s: %v", tag.Kind, tag.Value, quickstart.Name, err) |
Contributor
There was a problem hiding this comment.
Since we didn't have error handling prior it's hard to tell what the flow prior to these changes were (returning early or not). As long things are working by the end this should be fine, but something to think about.
Comment on lines
+536
to
+538
| errMsg := fmt.Sprintf("failed to save quickstart %s after adding tag: %v", quickstart.Name, err) | ||
| logrus.Error(errMsg) | ||
| result.addError(errMsg) |
Contributor
There was a problem hiding this comment.
This pattern of:
errMsg :=
logrus.Error
result.addError
Is duplicated a few times. I wonder if we can have a helper func that does it to reduce dupe?
| newTag.Type = models.TagType(tag.Kind) | ||
| newTag.Value = tag.Value | ||
|
|
||
| r := DB.Preload("HelpTopics").Where("type = ? AND value = ?", models.TagType(newTag.Type), newTag.Value).Find(&originalTag) |
pkg/database/db_seed.go
Outdated
| } | ||
|
|
||
| SeedFavorites(favorites) | ||
| func SeedTags() error { |
Contributor
There was a problem hiding this comment.
Is SeedTags appropriate for this func? It seems to be doing more than just tags
Contributor
|
It doesn't seem changing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
Summary by Sourcery
Refactor and enhance the database seeding process to preserve complete QuickStart data structures, standardize file and database operations with new helpers, implement a phased workflow with detailed logging and error tracking, and add verification tests to ensure content integrity.
New Features:
Enhancements:
Documentation:
Tests: