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

pkg/cincinnati: update unamrshal to node to match the result from cincinnati #113

Merged

Conversation

abhinavdahiya
Copy link
Contributor

@abhinavdahiya abhinavdahiya commented Feb 5, 2019

Based on cincinnati graph api Response the fields for a node must be version, image and metadata. But response from current api:

$ curl --silent --header 'Accept:application/json' https://api.openshift.com/api/upgrades_info/v1/graph | jq .
{
  "nodes": [
    {
      "version": "4.0.0-5",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-5",
      "metadata": {}
    },
    {
      "version": "4.0.0-4",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-4",
      "metadata": {}
    },
    {
      "version": "4.0.0-6",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6",
      "metadata": {}
    },
    {
      "version": "4.0.0-7",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-7",
      "metadata": {}
    },
    {
      "version": "4.0.0-8",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-8",
      "metadata": {}
    },
    {
      "version": "4.0.0-9",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-9",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.1",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1",
      "metadata": {
        "description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build"
      }
    },
    {
      "version": "4.0.0-0.okd-0",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.okd-0",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.2",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.2",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.3",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.3",
      "metadata": {}
    }
  ],
  "edges": [
    [
      1,
      0
    ],
    [
      0,
      2
    ],
    [
      2,
      3
    ],
    [
      3,
      4
    ],
    [
      4,
      5
    ],
    [
      6,
      8
    ],
    [
      8,
      9
    ]
  ]
}

has fields version, payload and metadata for a node.

3956b76 changed the previouly Payload field to Image creating invalid node object.

With this PR, we are using the unmarshalling version and payload fields for a node.

/cc @crawford @smarterclayton

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 5, 2019
@crawford
Copy link
Contributor

crawford commented Feb 6, 2019

According to openshift/cincinnati@5d226da, this should have been "fixed" already. Personally, I'm in favor of reverting that change and keeping this one.

@abhinavdahiya abhinavdahiya force-pushed the fix_available branch 2 times, most recently from 0c8b59c to 4a6aad8 Compare February 6, 2019 00:20
@abhinavdahiya
Copy link
Contributor Author

abhinavdahiya commented Feb 6, 2019

Running this locally

$ oc get clusterversion -oyaml
apiVersion: v1
items:
- apiVersion: config.openshift.io/v1
  kind: ClusterVersion
  metadata:
    creationTimestamp: 2019-02-06T00:51:12Z
    generation: 1
    name: version
    namespace: ""
    resourceVersion: "956"
    selfLink: /apis/config.openshift.io/v1/clusterversions/version
    uid: 4cbc79c7-29a9-11e9-b85c-664f163f5f0f
  spec:
    channel: fast
    clusterID: a7d11204-7193-41ca-ad4a-337a540a5dac
    upstream: https://api.openshift.com/api/upgrades_info/v1/graph
  status:

    availableUpdates:
    - image: quay.io/openshift-release-dev/ocp-release:4.0.0-6
      version: 4.0.0-6

    conditions:
    - lastTransitionTime: 2019-02-06T00:51:21Z
      status: "False"
      type: Available
    - lastTransitionTime: 2019-02-06T00:53:21Z
      status: "False"
      type: Failing
    - lastTransitionTime: 2019-02-06T00:51:21Z
      message: 'Working towards 0.0.1-2019-02-06-003220: 22% complete'
      status: "True"
      type: Progressing
    - lastTransitionTime: 2019-02-06T00:51:21Z
      status: "True"
      type: RetrievedUpdates
    desired:
      image: quay.io/abhinavdahiya/origin-release@sha256:4e9f3686380d26566e9112fdd53262068004a22a85f93111a0f10d0b3dfdc3a9
      version: 0.0.1-2019-02-06-003220
    history:
    - completionTime: null
      image: quay.io/abhinavdahiya/origin-release@sha256:4e9f3686380d26566e9112fdd53262068004a22a85f93111a0f10d0b3dfdc3a9
      startedTime: 2019-02-06T00:51:21Z
      state: Partial
      version: 0.0.1-2019-02-06-003220
    observedGeneration: 1
    versionHash: G0evldCtMHI=
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

this returns list of available versions

    availableUpdates:
    - image: quay.io/openshift-release-dev/ocp-release:4.0.0-6
      version: 4.0.0-6

@smarterclayton
Copy link
Contributor

I’m not super happy about a string called payload. If Cincinnati has more structured data, that’s one thing (payload: {image: }}). Payload string isn’t really future proof or correct.

@openshift-merge-robot
Copy link
Contributor

/retest

1 similar comment
@openshift-merge-robot
Copy link
Contributor

/retest

@abhinavdahiya
Copy link
Contributor Author

/hold

need to make sure we have finalized the cincinnati openshift's response api

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 6, 2019
@lucab
Copy link
Contributor

lucab commented Feb 7, 2019

cincinnati counterpart of this is openshift/cincinnati#56.

I'd be happy if we can get the two green-stamped at the same time, ensuring client and server stop trying to re-define the cincinnati protocol independently.

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2019
@abhinavdahiya
Copy link
Contributor Author

cincinnati counterpart of this is openshift/cincinnati#56.

I'd be happy if we can get the two green-stamped at the same time, ensuring client and server stop trying to re-define the cincinnati protocol independently.

openshift/cincinnati#56 was merged

/hold cancel

ping @smarterclayton @crawford

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2019
…cinnati

Based on [1] the fields for a node must be `version`, `image` and `metadata`. But response from current api:

```console
$ curl --silent --header 'Accept:application/json' https://api.openshift.com/api/upgrades_info/v1/graph | jq .
{
  "nodes": [
    {
      "version": "4.0.0-5",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-5",
      "metadata": {}
    },
    {
      "version": "4.0.0-4",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-4",
      "metadata": {}
    },
    {
      "version": "4.0.0-6",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-6",
      "metadata": {}
    },
    {
      "version": "4.0.0-7",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-7",
      "metadata": {}
    },
    {
      "version": "4.0.0-8",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-8",
      "metadata": {}
    },
    {
      "version": "4.0.0-9",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-9",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.1",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1",
      "metadata": {
        "description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build"
      }
    },
    {
      "version": "4.0.0-0.okd-0",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.okd-0",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.2",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.2",
      "metadata": {}
    },
    {
      "version": "4.0.0-0.3",
      "payload": "quay.io/openshift-release-dev/ocp-release:4.0.0-0.3",
      "metadata": {}
    }
  ],
  "edges": [
    [
      1,
      0
    ],
    [
      0,
      2
    ],
    [
      2,
      3
    ],
    [
      3,
      4
    ],
    [
      4,
      5
    ],
    [
      6,
      8
    ],
    [
      8,
      9
    ]
  ]
}
```
has fields `version`, `payload` and `metadata` for a node.

3956b76 changed the previouly `Payload` field to `Image` creating invalid node object when fetching updates from cincinnati.

With this PR, we are using the unmarshalling `version` and `payload` fields for a node.

[1]: https://github.com/openshift/cincinnati/blob/3616af287436ebe3b47262cbd1d32ad7e238a277/docs/design/cincinnati.md#response
@smarterclayton
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [abhinavdahiya,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 00b7e79 into openshift:master Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants