Screwdriver: [Feature request] Mistake of annotations is hardly noticeable

[yoshwata]

What happened

An example of screwdriver.yaml is below.

shared:
  annotations:
    screwdriver.cd/chainPR: true
    screwdriver.cd/cpu: MICRO

jobs:
  job01:
    image: node:10
    requires: [ ~pr, ~commit ]
    steps:
      - echo: echo hoge
  job02:
    image: node:10
    requires: [ job01 ]
    steps:
      - echo: echo hoge

chainPR is Pipeline-Level Annotations, so chainPR is not configured to the pipeline becaseu it is the shared setting.
The user can find it after pipeline starts because annotation is free format object, and validator does not warn it as invalid.

What you expected to happen

The user can find annotation's mistake from feedback of UI.

How to reproduce it

Starts the pipeline of which screwdriver.yaml is below

shared:
  annotations:
    screwdriver.cd/chainPR: true
    screwdriver.cd/cpu: MICRO

jobs:
  job01:
    image: node:10
    requires: [ ~pr, ~commit ]
    steps:
      - echo: echo hoge
  job02:
    image: node:10
    requires: [ job01 ]
    steps:
      - echo: echo hoge

chainPR is not configured, but no one warns about it.

relevant

Validator should validate buildPeriodically syntax · Issue #1611 · screwdriver-cd/screwdriver
Settings should be namespaced · Issue #534 · screwdriver-cd/screwdriver
Support Annotations for Feature Configuration · Issue #580 · screwdriver-cd/screwdriver

[yoshwata]

These are just ideas to solve this issue.

Solution1

Add validation to annotation and jobs.job.annotation. Now there is no validation to do.

Advantage

• Simple
• It can be error when it should be

Disadvantage

• annotation cannot be used as user free area.
• We might have to think about rename or rearrangement of keys of annotation. Is the key name annotation proper even if it was validated?
• Many people have to migrate if the key name was changed.

Solution2

Define dictionary of reserved word, and compare it with the key of jobs.annotations of screwdriver.yaml. If the key is proper as reserved word but is not as the key of jobs.annotation (e.g. chainPR is set as jobs.annotation), validator warns for user.

Advantage

• Users can use annotations as free field as before.
• No spec changes, so no migration.

Disadvantage

• It has to be warn state for living screwdriver.cd/* and free area together
• Maintenance of the reserved word table.
• The design seems to be messy.

Solution3

Write annotation.json on build, and users can see how the value was set after build.
annotation.json is like below

pipeline:
  settings:
    screwdriver.cd/restrictPR: true // <- valid settings
job:
  jobA:
    settings:
      screwdriver.cd/cpu: HIGH // <- valid settings
    data:
      screwdriver.cd/chainPR: true // <- Users can see the value is set just as data
      foo:
        bar:
          buz: "12345"

It is important to show both valid settings and the data.

Advantage

• There is no discomforts as a design.

Disadvantage

• Users might not see Artifacts tab, so it's better to show on UI(but there is no idea to show nested key-value on a Screen.)

[yoshwata]

I prefer to solution1 and it should be done until key name change stepwisely. For example,

1. Make new alternative fields like jobs.spec.cpu for all of annotations words. The new fields are validated strictly.
2. Notify users to migrate it.
3. Abolish annotations.* and it starts to work as just an free key-value field.

Which is prefer for you? Or any ideas?

[kumada626]

I think that free fields should leave in the settings. And I don't think users should be forced to migrate ineffectively. So I prefer to just warning when it was set in a position that wasn't supposed to be like solution2.

[wahapo]

I think solution2 is more desirable.
In addition to giving a warning in case of suspicion, you can also think of emphasizing it when it is correct. By setting a specific key such as annotations.screwdriver.cd/cpu in the yaml, the behavior has now been changed secretly. If we highlight the changes in the build behavior so that users can clearly notice them, users will easily notice that it has made a mistake.

[yoshwata]

There are some points what I noticed from comments of @kumada626 and @wahapo.

Update solution1's advantage

It can be error when it should be

Update solution2's advantage

No spec changes, so no migiration.

Update solution2's disadvantage

It has to be warn state for living screwdriver.cd/* and free area together

[yoshwata]

Updated for solution3

[wahapo]

As for solution3, if the information is only necessary for debugging and is not important for the build result, I think that it is not a problem if you put it in artifacts even if it is not noticeable.
Finding and displaying unused settings is complicated. I prefer solution3 because it is simpler to display the settings used.

[yoshwata]

@jithine and VM member, I'm looking forward to get what you prefer.

[jithine]

I prefer solution 2. User ability to use annotations free form to generate yaml anchors for reusable steps & jobs must be maintained That is heavily used https://blog.screwdriver.cd/post/189301069397/yaml-tip-using-anchors-for-shared-steps-jobs

We don't have a dictionary of valid annotations. each business logic picks up what it wants and then use it. We should maintain one, and in validator, we can simply filter out all valid SD provided annotations and check if it's value is correct and if it's provided in correct context of pipeline or job level annotations.

If value is invalid.

1. we can simply return a placeholder to denote that it's an invalid value, and let UI handle display in sd-setup-init or in a custom step. Everywhere the value is accessed should treat this invalid value and not default value.
   
   
   
   1. Alternatively throw config parse error, but this will cause user builds to fail.
   
   

[wahapo]

If the users make a mistake, it can be intentional, so the build should not fail. Even if we warn users of an invalid value at some step, users may not notice it. I think it is effective to let the user notice the mistake in writing with validator. It would be great if the validator could display effectively just returning an empty placeholder if it's an invalid. I think it is necessary to display valid values so that they stand out.

[yoshwata]

@jithine @wahapo
I might misunderstand what your placeholders means, but I guess it as data like {} and proceed discussion.

This is steps to notify the setting is valid or not to users.

1. SD build sends their annotation placeholders to parser or validator.
2. Parser returns placeholders to SD build, but All of them are accepted as valid annotation.
3. SD UI shows the accepted annotations as build log.
4. Finally users can check it was valid or not.

I don't choose the method treating it as error or warn because it might be complicated to compare unevaluated placeholders and evaluated placeholders.

[yoshwata]

@jithine @wahapo Does this match your understanding of placeholders?

[wahapo]

We(YJ Team) had a direct discussion. I think it's perfect.

[yoshwata]

This is implemented. Thanks @wahapo @ibu1224 !!