search

kubernetes / test-infra

Test infrastructure for the Kubernetes project.
Apache License 2.0
3.83k stars 2.65k forks source link

Some prowjob annotations are formatted confusingly #13413

Closed spiffxp closed 3 years ago

spiffxp commented 5 years ago

What should be cleaned up or changed: Prowjobs that had annotations added to them have them positioned in a place that makes little sense to a human: https://github.com/kubernetes/test-infra/pull/13205#issuecomment-509814928

Provide any links for context: Ideally I would want annotations right after the job name, this is what I listed in examples: https://github.com/kubernetes/test-infra/tree/master/config/jobs#job-examples

presubmits:
  kubernetes/community:
  - name: pull-community-verify  # convention: (job type)-(repo name)-(suite name)
    annotations:
      testgrid-dashboards: sig-contribex-community
      testgrid-tab-name: pull-verify
    branches:
    - master
    decorate: true
    always_run: true
    spec:
      containers:
      - image: golang:1.12.5
        command:
        - /bin/bash
        args:
        - -c
        # Add GOPATH/bin back to PATH to workaround #9469
        - "export PATH=$GOPATH/bin:$PATH && make verify"

Compare to what the job actually is right now:

presubmits:
  kubernetes/community:
  - name: pull-community-verify
    branches:
    - master
    always_run: true
    decorate: true
    spec:
      containers:
      - image: golang:1.12.5
        command:
        - sh
        - "-c"
        # Add GOPATH/bin back to PATH to workaround #9469
        - "export PATH=$PATH:$GOPATH/bin && make verify"
    annotations:
      testgrid-dashboards: sig-contribex-community
      testgrid-tab-name: pull-verify

I would honestly be fine with a one-shot that fixes this. The rabbit hole of opinions on field ordering with yaml can be deep. But that also an option if we feel that strongly. I have found that go-yaml likes to serialize fields in struct order. sigs.k8s.io/yaml only understands alpha order. ruamel.yaml will preserve order when parsing, but I'm not entirely sure what re-ordering looks like there.

FYI @chases2 @Katharine

/area jobs /help

k8s-ci-robot commented 5 years ago

@spiffxp: This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to [this](https://github.com/kubernetes/test-infra/issues/13413): >**What should be cleaned up or changed**: >Prowjobs that had annotations added to them have them positioned in a place that makes little sense to a human: https://github.com/kubernetes/test-infra/pull/13205#issuecomment-509814928 > >**Provide any links for context**: >Ideally I would want annotations right after the job name, this is what I listed in examples: https://github.com/kubernetes/test-infra/tree/master/config/jobs#job-examples >```yaml >presubmits: > kubernetes/community: > - name: pull-community-verify # convention: (job type)-(repo name)-(suite name) > annotations: > testgrid-dashboards: sig-contribex-community > testgrid-tab-name: pull-verify > branches: > - master > decorate: true > always_run: true > spec: > containers: > - image: golang:1.12.5 > command: > - /bin/bash > args: > - -c > # Add GOPATH/bin back to PATH to workaround #9469 > - "export PATH=$GOPATH/bin:$PATH && make verify" >``` > >Compare to what the job actually is right now: >```yaml >presubmits: > kubernetes/community: > - name: pull-community-verify > branches: > - master > always_run: true > decorate: true > spec: > containers: > - image: golang:1.12.5 > command: > - sh > - "-c" > # Add GOPATH/bin back to PATH to workaround #9469 > - "export PATH=$PATH:$GOPATH/bin && make verify" > annotations: > testgrid-dashboards: sig-contribex-community > testgrid-tab-name: pull-verify >``` > >I would honestly be fine with a one-shot that fixes this. The rabbit hole of opinions on field ordering with yaml can be deep. But that also an option if we feel that strongly. I have found that go-yaml likes to serialize fields in struct order. sigs.k8s.io/yaml only understands alpha order. ruamel.yaml will preserve order when parsing, but I'm not entirely sure what re-ordering looks like there. > >FYI @chases2 @Katharine > >/area jobs >/help Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
spiffxp commented 5 years ago

/sig testing

pjh commented 5 years ago

I'll chime in, I just noticed the annotations: in the test config file that I care about (https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-cloud-provider/gcp/sig-gcp-windows.yaml) and I spent ten confused minutes before I realized that these annotations don't apply to the job they're on top of but to the job above them, by which they confusingly are separated by a newline :/

e.g. this annotation does not apply to ci-kubernetes-e2e-windows-gce, but the job above it:

      ...
      resources:
        requests:
          memory: "8Gi"

  annotations:
    testgrid-dashboards: google-windows
    testgrid-tab-name: windows-prototype
- name: ci-kubernetes-e2e-windows-gce
  decorate: true
  ...
fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot commented 4 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

matthyx commented 4 years ago

@spiffxp do you still need some help on this issue?

spiffxp commented 4 years ago

@matthyx yes AFAIK formatting is still inconsistent and confusing

matthyx commented 4 years ago

/remove-lifecycle rotten /assign

matthyx commented 4 years ago

@spiffxp the issue is kinda hairy, so as I see it we have this chain of calls: mkpj.main -> "sigs.k8s.io/yaml".Marshal "sigs.k8s.io/yaml".Marshal -> "gopkg.in/yaml.v2".JSONToYAML

However "gopkg.in/yaml.v2".JSONToYAML calls Unmarshal and then Marshal with a struct{} which from my test is alone sufficient to lose the "order" in map items.

I will check if it's possible to use directly "gopkg.in/yaml.v2".Marshal to keep the order.

matthyx commented 4 years ago

Wait... now I realize I might have not understood the issue...

@spiffxp apparently you only want to correct the 184 files and move annotations just after the name they belong to? Like here: https://github.com/kubernetes/test-infra/compare/master...matthyx:annotations?expand=1

fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

matthyx commented 4 years ago

/remove-lifecycle stale

fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

matthyx commented 4 years ago

/remove-lifecycle stale

fejta-bot commented 4 years ago

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle stale

fejta-bot commented 3 years ago

Stale issues rot after 30d of inactivity. Mark the issue as fresh with /remove-lifecycle rotten. Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /lifecycle rotten

matthyx commented 3 years ago

@spiffxp do we still want to go further? I need to come back to it and integrate some checks on the yamls.

fejta-bot commented 3 years ago

Rotten issues close after 30d of inactivity. Reopen the issue with /reopen. Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta. /close

k8s-ci-robot commented 3 years ago

@fejta-bot: Closing this issue.

In response to [this](https://github.com/kubernetes/test-infra/issues/13413#issuecomment-755266138): >Rotten issues close after 30d of inactivity. >Reopen the issue with `/reopen`. >Mark the issue as fresh with `/remove-lifecycle rotten`. > >Send feedback to sig-testing, kubernetes/test-infra and/or [fejta](https://github.com/fejta). >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.