- 
                Notifications
    You must be signed in to change notification settings 
- Fork 554
feat: custom tag #3847
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
feat: custom tag #3847
Conversation
| EntityKey int `json:"entityKey"` | ||
| EntityValue string `json:"entityValue"` | ||
| TagPattern string `json:"tagPattern"` | ||
| AutoIncreasingNumber int `json:"counterX"` | 
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.
can be used better name for counterX
        
          
                pkg/CustomTagService.go
              
                Outdated
          
        
      | ) | ||
|  | ||
| const ( | ||
| imagePathPattern = "%s/%s:%s" // dockerReg/dockerRepo:Tag | 
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.
we can put these type of constants at some common place
        
          
                pkg/CustomTagService.go
              
                Outdated
          
        
      | customTagData := repository.CustomTag{ | ||
| EntityKey: tag.EntityKey, | ||
| EntityValue: tag.EntityValue, | ||
| TagPattern: strings.ReplaceAll(tag.TagPattern, "{X}", "{x}"), | 
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.
can we use some other relevant name than X
        
          
                pkg/CustomTagService.go
              
                Outdated
          
        
      | if customTagData.AutoIncreasingNumber < 0 { | ||
| return "", fmt.Errorf("counter {x} can not be negative") | ||
| } | ||
| dockerImageTag := strings.ReplaceAll(customTagData.TagPattern, "{x}", strconv.Itoa(customTagData.AutoIncreasingNumber-1)) //-1 because number is already incremented, current value will be used next time | 
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.
x
        
          
                pkg/CustomTagService.go
              
                Outdated
          
        
      |  | ||
| // for patterns like v1.0.{x} we will calculate count with . in {x} i.e .{x} | ||
| variableCount := 0 | ||
| variableCount = variableCount + strings.Count(customTagPattern, "{x}") | 
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.
can we use some other placeholder everywhere for x
| } | ||
| err = impl.customTagService.DeactivateImagePathReservation(ciWorkflow.ImagePathReservationId) | ||
| if err != nil { | ||
| impl.Logger.Errorw("unable to update ci workflow, its eligible to mark failed", "err", err) | 
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.
why not returning 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.
there are more than one ci_workflows updated in this function. If there is some error with one workflow then we are skipping only that workflow and continuing our loop
| return err | ||
| } | ||
|  | ||
| go func() { | 
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.
can use use nats for this purpose
| Kudos, SonarCloud Quality Gate passed!     
 
 | 








feat: custom tag
Fixes #3566
How Has This Been Tested?
Checklist:
Does this PR introduce a user-facing change?
Yes