Inheritance methods of Metadata is not intuitive

[yakanechi]

What happened:
If there are multiple trigger sources for the current metadata, the value of the job that completed first takes precedence.
In the following example, the value is set in job1 and then reset in job3, but job2 is completed before job3, so the value of x is 1 in job4.
It seems strange that values set later are not used.

What you expected to happen:
Consider adding a timestamp-like feature for each metadata, with the newly added ones taking precedence.

How to reproduce it:

jobs:
  Job1:
    requires: [ ~commit ]
    steps:
      - meta set x "1"

  Job2:
    requires: [ ~Job1 ]
    steps:
      - echo "nothing"

  Job3:
    requires: [ ~Job1 ]
    steps:
      - sleep 60
      - meta set x "2"

  Job4:
    requires: [ Job2, Job3 ]
    steps:
      - echo $(meta get x)

[VonnyJap]

@sagar1312 @tkyi - do you guys have any thoughts on how to solve this issue? Basically this looks like a bug, x should be seen as 2 in Job4, but users are seeing the old value 1 returned.

[tkyi]

Yeah that sounds like a bug. Should be the more recently finished build that takes precedence.

[VonnyJap]

@yakanechi - I was able to reproduce this issue.
https://cd.screwdriver.cd/pipelines/15511/events/823582
https://github.com/VonnyJap/screwdriver-config/blob/main/test-meta/screwdriver.yaml

I believe this can be fixed with this implementation below:
Sort by build endtime is performed prior to the merge of all parent builds metadata.

Will your side be able to make a launcher change on this?

[yakanechi]

@VonnyJap
Yes, we plan to fix this in the near future.

[kumada626]

Only sort parent builds by build endtime not solve below case.

jobs:
  Job1:
    requires: [ ~commit ]
    steps:
      - meta set x "1"

  Job2:
    requires: [ ~Job1 ]
    steps:
      - meta set x "2"

  Job3:
    requires: [ ~Job1 ]
    steps:
      - sleep 60
      - echo "nothing"

  Job4:
    requires: [ Job2, Job3 ]
    steps:
      - echo $(meta get x)

In that case, x should be seen as 2 in Job4, but users are seeing the old value 1 returned.
Because x is set in Job1 and is carried over to Job3, when the meta of Job3 is merged, x is also overwritten with the value of Job3.
I think that something like a timestamp per metadata key might be needed to solve this problem.

[VonnyJap]

@kumada626 - I got your point now.
So yeah, we need to implement a more robust solution around metadata, including whether or not metadata is dirty (being changed) or clean (no change) after passing through a certain build. During the merging, this information along with timestamp is needed to decide the latest value of a certain metadata key. This is going to involve a larger design change, which we need to partner to do together.

As such, I think the PR raised here can be put on hold until we make the larger design change.
screwdriver-cd/launcher#481

Please let me know what your thoughts are.

[kumada626]

@VonnyJap I agree that large design changes are needed.
However, at least at this point in time, the order of builds to merge the metadata will be unique, so that PR change can be merged before the design change.

[VonnyJap]

@y-oksaku - The condition requiring isJoin to be true only when parentBuilds is greater than 1, as outlined here, results in external metadata merging being skipped when parentBuilds refers to an external build with only one parent build. Could you please take a look?

[y-oksaku]

@VonnyJap
Yes, external metadata is not merged when parentBuilds refers to only one external build.

However, this condition was not introduced by screwdriver-cd/launcher#481 . It has been this way since before this modification.
https://github.com/screwdriver-cd/launcher/blob/8e928b64cc055140b6694bcf79a1de673f90ebfc/launch.go#L317-L329

I think that it is supposed to use external flag as meta get foo --external sd@123:main to get external build's metadata.

[VonnyJap]

@y-oksaku - I have attempted to relax the condition with this change, screwdriver-cd/launcher#488. What do you think? If we are in agreement, I will make an additional commit to fix the test.

[y-oksaku]

@VonnyJap - I agree this change 👍
There are no reasons to skip merging metadata from parent builds. It gets cleaner.