-
Notifications
You must be signed in to change notification settings - Fork 469
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
enhancements/update/available-update-metadata: 'url' docstring context #408
enhancements/update/available-update-metadata: 'url' docstring context #408
Conversation
91c60e8
to
a46ca76
Compare
I've pushed 91c60e8 -> a46ca76 removing the:
sentence, because it conflicts with the enhancement proposal's:
implementation. |
a46ca76
to
b22d4e6
Compare
I've pushed a46ca76 -> b22d4e6 adding this updated sentence about the |
// of no more-specific resource addressing your question for this | ||
// release image. For example, this could link errata or other | ||
// release notes describing the release image. | ||
// release image. This URL is set by the 'url' metadata property on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go here if you know of no more-specific resource addressing your question for this of no more-specific resource addressing your question for this
makes the meaning of the comment odd and confusing. Why do we need to say this here? It does not seem like a necessary sentence because the sentences before it and after it communicates things in a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As described here, my resistance to url
, and preference for homepage
, revolved around url
being generic, with no clear semantic meaning. Clayton's argument was that the semantics are that this property references the most-generic resource scoped to this release, i.e. that it is a good choice regardless of which released-scoped information you are seeking. I want some sort of most-generic statement like my "no more-specific resource" line to get this across. For example, if you sometimes know of another release-scoped resource (e.g., an entry for this release in a changelog or release notes), falling back to the more-generic url
when you cannot determine your specific link would be perfectly acceptable, and not a hack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking Do you have a concrete example of where this might link to? For instance, for 4.5.2, where would url
take you?
For example, if you sometimes know of another release-scoped resource (e.g., an entry for this release in a changelog or release notes), falling back to the more-generic url when you cannot determine your specific link would be perfectly acceptable, and not a hack.
The language is vague enough in the API doc that it's not clear to me when to use something other than url
. Console always links to the release notes today. Would that sometimes be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the concern is that the link won't have detailed release notes, is it enough simply to say that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a concrete example of where this might link to?
The most-generic semantics mean that you shouldn't need a concrete example. Whatever they link to should be generic enough for any release-scoped use case. But practically, these will continue to link the errata pages for OCP releases, and release-controller detail pages or some such for FCOS releases.
The language is vague enough in the API doc that it's not clear to me when to use something other than url.
We want the language to clearly communicate:
- If you know of a specific link for your purpose, by all means, use that.
- Regardless of whether you know of a specific link for your release-scoped purpose, using the
url
value is fine. It is the most-generic release-scoped link there is, so it will never be wrong. The worst it can be is "not as specific as $ALTERNATIVE".
Console always links to the release notes today. Would that sometimes be better?
So yes, ignoring this value and linking to the release notes (when you know where they'll be) is fine. And you should fall back to this value for the "release notes link" use-case when you are not sure of a more-specific release notes target. We want the API docs to make that clear without folks having to ask for clarification out of band. Can you float some alternative wording if the current proposal here does not express this clearly?
If the concern is that the link won't have detailed release notes, is it enough simply to say that?
If detailed release notes exist, the url
value should either link to them or a more-generic page that in turn links to the release notes (OCP errata link to release notes today). If detailed release notes exist and the url
value links to a page from which the release notes are not discoverable, that would be a bug we could file against either ART (who are deciding to link the errata page with url
) or the Errata folks (who are deciding what content to put on the errata page).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, it's more helpful to give examples of what the page is...
I'm ok with giving examples. But I think we're doing it wrong if the only way we can describe the intended values is by saying "it's going to be content like $existing_examples". We should be able to describe the content in a way that stands alone, and then supplement that normative definition with informative examples.
I don't know that you need to give clients permission to use another link. People will do that anyway if they want :) And I wouldn't assume the link is ever "wrong."
If we had URI properties for license
and releaseNotes
, using the value of releaseNotes
and saying "license is here" when license
was unset would be clearly wrong. The special thing about the most-generic url
is that saying "license is here" and linking the the value of url
would be perfectly fine, because you could assume that the license information would be easily discoverable from the generic url
target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored:
For example, this could link errata or other release notes describing the release image.
with b22d4e6 -> 73a1139, to address @spadgett's "Do you have a concrete example of where this might link to?".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this language. Errata is confusing to non OCP customers, release notes is confusing to OCP customers. This sentence is not a good sentence for those reasons.
The URL will contain information about the release.
is the minimum bar. I am fine with leaving OCP or OKD language out right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with leaving OCP or OKD language out right now.
Ok, back to b22d4e6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b22d4e6
to
73a1139
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/hold
for other feedback
73a1139
to
b22d4e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
b22d4e6
to
d859e06
Compare
@@ -71,15 +74,16 @@ type Release struct { | |||
// +required | |||
Image string `json:"image"` | |||
|
|||
// URL describes the contents of this release and will include | |||
// references to other resources as necessary. Go here if you know | |||
// url contains information about the release. Go here if you know |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the Go here if you know of no more-specific resource
. It is not useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d859e06
to
c49fab6
Compare
Fixups to 2eb16cf (enhancements/update/available-update-metadata: Propose a new enhancement, 2019-11-19, openshift#123): * Including two sentences from Clayton about how the CVO sources this information and how we build CI/nightly images [1,2]. I think these two sentences are poking holes in the abstraction that the CVO is committing to with the new API, and that consumers should not have to know or care about these implementation details. Clayton feels like they provide useful context about how the system works, and that we'll adjust them as necessary if we ever have cause to alter the implementation. * Downcasing in the Go comments, to match the JSON property, following the Kubernetes API pattern. * 'despire' -> 'despite' typo fix. [1]: openshift/api#521 (comment) [2]: openshift/api#521 (comment)
c49fab6
to
12eff48
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sdodson, smarterclayton, spadgett, wking 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:
Approvers can indicate their approval by writing |
This commit adds the ability to store release metadata, implementing openshift/enhancements@2eb16cf80d (enhancements/update/available-update-metadata: Propose a new enhancement, 2020-07-20, openshift/enhancements#123) as adjusted by openshift/enhancements@12eff48079 (enhancements/update/available-update-metadata: 'url' docstring context, 2020-07-21, openshift/enhancements#408). For example the list of channels that a release is in: $ curl -sH 'Accept:application/json' 'https://api.openshift.com/api/upgrades_info/v1/graph?channel=stable-4.2' | jq -r '.nodes[] | select(.version == "4.2.4").metadata["io.openshift.upgrades.graph.release.channels"] | split(",")[]' candidate-4.2 fast-4.2 stable-4.2 That will allow the console and other downstream tooling to expose a list of channels available to a given cluster right now, instead of hard-coding an expected list [1]. This will account for things like phased releases, where we may not recommend an upgrade for your particular stable-4.2 cluster even though we are recommending the upgrade for other stable-4.2 clusters. Pivoting around this retyping in Go will require consumer bumps, but I don't think it's a breaking change because the only YAML removal is 'force', which is meaningless in AvailableUpdates. So I don't think this needs to count as a backwards-compat-breaking schema bump. Autogenerated bumps via: $ unset GOBIN # vendor/k8s.io/code-generator/generate-groups.sh and such require "${GOPATH}/bin/deepcopy-gen" $ go get k8s.io/gengo/examples/deepcopy-gen $ hack/update-deepcopy.sh $ hack/update-swagger-docs.sh $ make update-codegen-crds [1]: openshift/console#2935
Fixups to 2eb16cf (#123):