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

fix(3125): merge parent build meta order by end time #481

Conversation

y-oksaku
Copy link
Contributor

Context

Currently, if the parent builds have same key metadata, the value of the build completed earlier is used.
(Not value of the build completed later)

This behavior is a little strange.

Objective

Sort merging parent builds metadata order by their finished time.

References

screwdriver-cd/screwdriver#3125 (comment)

License

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@y-oksaku y-oksaku marked this pull request as ready for review November 14, 2024 05:40
launch.go Outdated
parentBuild, err := api.BuildFromID(parentBuildID)
if err != nil {
return resultMeta, fmt.Errorf("Fetching Parent Build ID %d: %v", parentBuildID, err)
var join = len(parentBuildIds) > 1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var join = len(parentBuildIds) > 1
var isJoin = len(parentBuildIds) > 1

launch.go Outdated
Comment on lines 332 to 358
// Write to "sd@123:component.json", where sd@123:component is the triggering job
externalMetaFile := "sd@" + strconv.Itoa(parentJob.PipelineID) + ":" + parentJob.Name + ".json"
writeMetafile(metaSpace, externalMetaFile, metaLog, parentBuild.Meta)
if join {
marshallValue, err := json.Marshal(parentBuild.Meta)
if err != nil {
return resultMeta, fmt.Errorf("Cloning meta of Parent Build ID %d: %v", parentBuild.ID, err)
}
var externalParentBuildMeta map[string]interface{}
json.Unmarshal(marshallValue, &externalParentBuildMeta)

// Always exclude parameters from external meta
delete(externalParentBuildMeta, "parameters")

// delete local version of external meta
pIDString := strconv.Itoa(parentJob.PipelineID)
pjn := parentJob.Name
if sdMeta, ok := resultMeta["sd"]; ok {
if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok {
if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok {
delete(externalPipelineMeta.(map[string]interface{}), pjn)
resultMeta = deepMergeJSON(resultMeta, externalParentBuildMeta)
}

// delete local version of external meta
pIDString := strconv.Itoa(parentJob.PipelineID)
pjn := parentJob.Name
if sdMeta, ok := resultMeta["sd"]; ok {
if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok {
if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok {
delete(externalPipelineMeta.(map[string]interface{}), pjn)
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

please separate this logic to a different function, for example:

func handleExternalPipelineMeta(api screwdriver.API, parentBuild screwdriver.Build, parentJob screwdriver.Job, resultMeta map[string]interface{}, metaSpace, metaLog string, isJoin bool) (map[string]interface{}, error) {
    externalMetaFile := ExternalMetaFilePrefix + strconv.Itoa(parentJob.PipelineID) + ":" + parentJob.Name + ExternalMetaFileSuffix
    writeMetafile(metaSpace, externalMetaFile, metaLog, parentBuild.Meta)

    if isJoin {
        marshallValue, err := json.Marshal(parentBuild.Meta)
        if err != nil {
            return resultMeta, fmt.Errorf("Cloning meta of Parent Build ID %d: %v", parentBuild.ID, err)
        }
        var externalParentBuildMeta map[string]interface{}
        json.Unmarshal(marshallValue, &externalParentBuildMeta)
        delete(externalParentBuildMeta, "parameters")
        resultMeta = deepMergeJSON(resultMeta, externalParentBuildMeta)
    }

    pIDString := strconv.Itoa(parentJob.PipelineID)
    pjn := parentJob.Name
    if sdMeta, ok := resultMeta["sd"]; ok {
        if externalPipelineMeta, ok := sdMeta.(map[string]interface{})[pIDString]; ok {
            if _, ok := externalPipelineMeta.(map[string]interface{})[pjn]; ok {
                delete(externalPipelineMeta.(map[string]interface{}), pjn)
            }
        }
    }

    return resultMeta, nil
}

this will help the code to be more readable.

Copy link
Contributor Author

@y-oksaku y-oksaku Nov 19, 2024

Choose a reason for hiding this comment

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

Thank you for the suggestion. I fixed it 🙇

launch.go Outdated
resultMeta, err = handleExternalPipelineMeta(parentBuild, parentJob, resultMeta, metaSpace, metaLog, isJoin)

if err != nil {
return resultMeta, fmt.Errorf("Mergeing meta of External Parent Build ID %d: %v", parentBuild.ID, err)
Copy link
Contributor

@yk634 yk634 Nov 20, 2024

Choose a reason for hiding this comment

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

[nits] I think it's a spelling mistake.

Suggested change
return resultMeta, fmt.Errorf("Mergeing meta of External Parent Build ID %d: %v", parentBuild.ID, err)
return resultMeta, fmt.Errorf("Merging meta of External Parent Build ID %d: %v", parentBuild.ID, err)

launch.go Outdated
// SetExternalMeta checks if parent build is external and sets meta in external file accordingly
func SetExternalMeta(api screwdriver.API, pipelineID, parentBuildID int, mergedMeta map[string]interface{}, metaSpace, metaLog string, join bool) (map[string]interface{}, error) {
// setParentBuildsMeta checks if parent build is external and sets meta in external file accordingly
func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIds []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

[nits] In other places it is “parentBuildIDs”, so it might be better to be consistent.

Suggested change
func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIds []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) {
func setParentBuildsMeta(api screwdriver.API, pipelineID int, parentBuildIDs []int, mergedMeta map[string]interface{}, metaSpace, metaLog string) (map[string]interface{}, error) {

@VonnyJap
Copy link
Member

@y-oksaku - please rebase your branch and I will approve after that.

@VonnyJap VonnyJap merged commit dafc34c into screwdriver-cd:master Nov 23, 2024
2 checks passed
@y-oksaku y-oksaku deleted the fix-parent-build-meta-priority branch November 25, 2024 01:03
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.

4 participants