Skip to content

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Jul 10, 2020

Description of the change:

This PR reorganizes sdk.operatorframework.io to match the navigation
panel for olm.operatorframework.io, so that both the sites appear
to be part of one operator-framework family.

Motivation for the change:

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2020
@anik120 anik120 force-pushed the reorganise-doc-site branch from f7ec7c6 to 88fb147 Compare July 10, 2020 16:46
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 10, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @anik120,

really tks for the contribution. Ask we spoke in the slack. Could we not slit it by section to make the process work easy and fast? E.g ansible, golang an etc

Also, not that it is not passing in the CI. The changes performed here broke the links.

@jmrodri

This comment has been minimized.

@jmrodri
Copy link
Member

jmrodri commented Jul 10, 2020

Heading font size is much smaller than before and doesn't seem to match the one used on OLM either.
The font from this PR:
image

The old SDK fonts were much larger:
image

The OLM fonts seem to be slightly larger compared to the one used in this PR:
image

@jmrodri
Copy link
Member

jmrodri commented Jul 10, 2020

Core Tasks seems odd to me.
image

It seems to fit on the OLM side because the steps seem the same. But the SDK has the different types of operators which have their own audiences and presumably a different set of "core tasks".

@jmrodri
Copy link
Member

jmrodri commented Jul 10, 2020

Also, I think Getting Started should be called Installation on both sites since both of the docs just describe how to install SDK or OLM:

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main issue is with the Core Tasks menu seems odd for the SDK. The heading font is different size than the one on the OLM site, and I don't like the "Getting Started" nav title since it's really "Installing or Installation" that is being described.

@jmrodri
Copy link
Member

jmrodri commented Jul 10, 2020

I think I know why Core Tasks and Advanced Tasks seem odd. I think it should be Core Topics and Advanced Topics because on the SDK side they really aren't "tasks" per se.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2020
@camilamacedo86

This comment has been minimized.

@anik120

This comment has been minimized.

@anik120 anik120 force-pushed the reorganise-doc-site branch from 88fb147 to 802fb0e Compare July 14, 2020 21:02
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2020
@anik120 anik120 force-pushed the reorganise-doc-site branch from 802fb0e to bb4bbc1 Compare July 14, 2020 21:14
@anik120

This comment has been minimized.

@camilamacedo86

This comment has been minimized.

@anik120 anik120 force-pushed the reorganise-doc-site branch from bb4bbc1 to 464c157 Compare July 15, 2020 18:59
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2020
@anik120 anik120 force-pushed the reorganise-doc-site branch from 464c157 to 8b18644 Compare July 15, 2020 19:01
@camilamacedo86
Copy link
Contributor

Hi @anik120,

It still not passing in the CI. We cannot merge without that.

Regards the sanity check

  • You changed the CLI docs, however, they are generated automatically via the Makefile target make generate. So, you also need to change its scripts. See the CI issue in https://travis-ci.org/github/operator-framework/operator-sdk/jobs/708135863 and its script in ./hack/generate/cli-doc/gen-cli-doc.sh
  • You also need to run make generate locally. Then, the docs should be generated in the new path.
  • You can test it locally by running make test-sanity (note that all changes need to be committed because it will do a git diff at the end)

Regards the docs
It has a lot of broken links now. You need to fix the links ref in the docs. See: https://travis-ci.org/github/operator-framework/operator-sdk/jobs/708825906

@anik120 anik120 force-pushed the reorganise-doc-site branch from 1560c6c to 5f58a4b Compare July 16, 2020 18:57
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2020
@anik120 anik120 force-pushed the reorganise-doc-site branch 3 times, most recently from af0e585 to cb4ef64 Compare July 16, 2020 19:55
@anik120
Copy link
Contributor Author

anik120 commented Jul 16, 2020

@camilamacedo86 could you take a look now please. I've fixed most of it, there's a lot of external links that are broken. Not sure if I have to do anything about that in this PR.

@jmrodri
Copy link
Member

jmrodri commented Jul 17, 2020

tons of broken links because of all the churn happening in master with legacy being removed.

 * internally linking to docs/building-operators/golang/legacy/e2e-tests, which does not exist (line 1368) <a href="docs/building-operators/golang/legacy/e2e-tests">Writing E2E Tests</a> - /target/docs/building-operators/golang/legacy/e2e-tests/index.html * External link https://github.com/operator-framework/operator-sdk/blob/master/pkg/test/context.go failed: 404 No error * External link https://github.com/operator-framework/operator-sdk/blob/master/pkg/test/framework.go failed: 404 No error * External link https://github.com/operator-framework/operator-sdk/tree/master/pkg/test/e2eutil failed: 404 No error * internally linking to /docs/cli/legacy-cli/operator-sdk_test_local, which does not exist (line 1562) <a href="/docs/cli/legacy-cli/operator-sdk_test_local">SDK CLI Reference</a> - /target/docs/building-operators/golang/quickstart/index.html * External link https://v0-19-x.sdk.operatorframework.io/docs/building-operators/golang/legacy/quickstart/ failed: 404 No error - /target/docs/building-operators/helm/reference/watches/index.html * internally linking to /docs/helm/building-operators/reference/advanced_features/#passing-environment-variables-to-the-helm-chart, which does not exist (line 1363) <a href="/docs/helm/building-operators/reference/advanced_features/#passing-environment-variables-to-the-helm-chart">Using override values and passing environment variables to the Helm chart</a> * linking to internal hash #passing-environment-variables-to-the-helm-chart that does not exist (line 1363) <a href="/docs/helm/building-operators/reference/advanced_features/#passing-environment-variables-to-the-helm-chart">Using override values and passing environment variables to the Helm chart</a> - /target/docs/cli/operator-sdk_scorecard/index.html * internally linking to ../operator-sdk, which does not exist (line 1376) <a href="../operator-sdk">operator-sdk</a> - /target/docs/contribution-guidelines/release/index.html * internally linking to /docs/upgrade-sdk-version/backport-policy, which does not exist (line 1400) <a href="/docs/upgrade-sdk-version/backport-policy">backport policy</a> * internally linking to docs/installation/install-operator-sdk#prerequisites, which does not exist (line 1365) <a href="docs/installation/install-operator-sdk#prerequisites">prerequisites section</a> * internally linking to docs/installation/install-operator-sdk#prerequisites, which does not exist (line 1367) <a href="docs/installation/install-operator-sdk#prerequisites">prerequisites section</a> * internally linking to docs/installation/install-operator-sdk, which does not exist (line 1489) <a href="docs/installation/install-operator-sdk">installation guide.</a> * linking to internal hash #prerequisites that does not exist (line 1365) <a href="docs/installation/install-operator-sdk#prerequisites">prerequisites section</a> * linking to internal hash #prerequisites that does not exist (line 1367) <a href="docs/installation/install-operator-sdk#prerequisites">prerequisites section</a> - /target/docs/new-cli/operator-sdk_scorecard/index.html * internally linking to ../operator-sdk, which does not exist (line 1378) <a href="../operator-sdk">operator-sdk</a> - /target/docs/olm-integration/generating-a-csv/index.html * internally linking to /docs/advanced-topics/operator-capabilities/, which does not exist (line 1528) <a href="/docs/advanced-topics/operator-capabilities/">Operator maturity model</a> - /target/docs/overview/_overview/index.html * internally linking to /docs/advanced-topics/operator-capabilities/, which does not exist (line 1391) <a href="/docs/advanced-topics/operator-capabilities/">capability level documentation</a> * internally linking to /docs/faq/, which does not exist (line 1395) <a href="/docs/faq/">FAQ</a> - /target/docs/upgrading-sdk-version/v0.1.0-migration-guide/index.html * External link https://v0-19-x.sdk.operatorframework.io/docs/building-operators/golang/legacy/quickstart/#adding-3rd-party-resources-to-your-operator failed: 404 No error * External link https://v0-19-x.sdk.operatorframework.io/docs/building-operators/golang/legacy/quickstart/#build-and-run-the-operator failed: 404 No error * External link https://v0-19-x.sdk.operatorframework.io/docs/building-operators/golang/references/client/ failed: 404 No error 
@jmrodri
Copy link
Member

jmrodri commented Jul 17, 2020

And now there is a conflict so this PR needs a rebase

@anik120 anik120 force-pushed the reorganise-doc-site branch 5 times, most recently from d8022e8 to 2b366c4 Compare July 17, 2020 15:42
@anik120
Copy link
Contributor Author

anik120 commented Jul 17, 2020

@camilamacedo86 @jmrodri resolved all the broken links expect for these two and looks like they're broken in main branch too.

@anik120 anik120 force-pushed the reorganise-doc-site branch from 2b366c4 to d8e6a82 Compare July 17, 2020 17:32
@camilamacedo86
Copy link
Contributor

camilamacedo86 commented Jul 17, 2020

Hi @anik120,

See that it is not passing in the CI because of broken links. However, not that your PR/version is missing docs and it is not accurate. Something probably was wrong in your rebase.

To check the broking links see the error msg:

- /target/docs/cli/operator-sdk_scorecard/index.html * internally linking to ../operator-sdk, which does not exist (line 1194) <a href="../operator-sdk">operator-sdk</a> 

It means that the ../operator-sdk link in /docs/cli/operator-sdk_scorecard/_index.md do not exist. See:

https://github.com/operator-framework/operator-sdk/tree/2b366c4c969d2647fa61ea7b279f9c67e12e60fb/website/content/en/docs/cli

The same for the other scenario:

- /target/docs/new-cli/operator-sdk_scorecard/index.html * internally linking to ../operator-sdk, which does not exist (line 1196) <a href="../operator-sdk">operator-sdk</a> 

It means that the ../operator-sdk link in docs/new-cli/operator-sdk_scorecard/_index.md do not exist.

Also, see that something goes wrong in your rebase:

Screenshot 2020-07-17 at 18 31 42

The scorecard should be a menu with its docs instead that. Also, see that its cli index doc is also in the root when should not. You might need to review the changes made in the script to update the cli docs. See the master branch docs to check how it should be:

Screenshot 2020-07-17 at 18 33 42

@anik120 anik120 force-pushed the reorganise-doc-site branch 3 times, most recently from ecb59d6 to 8af3dca Compare July 17, 2020 19:39

**Note:** For Golang Operators this CLI has been deprecated. Please consult the [new CLI reference][new_CLI] doc.

[new_CLI]:/docs/cli/new-cli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect link:

Suggested change
[new_CLI]:/docs/cli/new-cli
[new_CLI]:/docs/new-cli

We might want to wait until #3455 is merged to simplify this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, we can merge #3455 after this one is merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estroz yea that'd be appreciated this PR has been in the works for a week now.
Although, CI job request is still pending for this one.


**Note:** This CLI reference is for the new Kubebuilder aligned CLI and project layout. See the [legacy CLI reference][legacy_cli] doc if using a legacy Golang project.

[legacy_CLI]:/docs/cli/legacy-cli
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[legacy_CLI]:/docs/cli/legacy-cli
[legacy_CLI]:/docs/cli
This PR reorganizes sdk.operatorframework.io to match the navigation panel for olm.operatorframework.io, so that both the sites appear to be part of one operator-framework family.
@anik120 anik120 force-pushed the reorganise-doc-site branch from 8af3dca to a33c70b Compare July 17, 2020 20:29
@jmrodri
Copy link
Member

jmrodri commented Jul 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2020
@camilamacedo86 camilamacedo86 dismissed their stale review July 17, 2020 22:06

shows that the changes requested was made

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shows OK for me 👍
Tks for your commitment with that 🥇
/lgtm

@camilamacedo86 camilamacedo86 merged commit 1248c1c into operator-framework:master Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged.

6 participants