search

kubeflow / pipelines

Machine Learning Pipelines for Kubeflow
https://www.kubeflow.org/docs/components/pipelines/
Apache License 2.0
3.61k stars 1.63k forks source link

Point pre-installed sample source links to permanent URIs #2721

Closed Ark-kun closed 4 years ago

Ark-kun commented 4 years ago

Otherwise the links stop working when the samples are moved.

Bobgy commented 4 years ago

It seems @numerology has already been doing this: https://github.com/kubeflow/pipelines/pull/2722

numerology commented 4 years ago

Summarize some discussion in #2722 here.

Pointing source links to permalinks can fix the 404 error for now, but it falls short in that our future update to READMEs and source codes won't be reflected in the preload samples in a timely manner.

One possible solution of that would be include some link checks in presubmit tests, to guarantee all the links in the docs and samples are not broken by the PR.

Ark-kun commented 4 years ago

it falls short in that our future update to READMEs and source codes won't be reflected in the preload samples in a timely manner.

But the source code would be correct (corresponding to the preloaded sample).

We can use version tag instead of commit hash to make it easier to update the samples config.

Another option might be to build/modify the config during the image building.

numerology commented 4 years ago

But the source code would be correct (corresponding to the preloaded sample).

Not exactly sure I understand... Are you suggesting there is a way to use permalink while keep the source code up-to-date at the same time?

Bobgy commented 4 years ago

Another option might be to build/modify the config during the image building.

This sounds like a good idea to me.

Bobgy commented 4 years ago

Adjusting on that

  1. Pin sample links to current commit SHA during image build
  2. Add a presubmit test using a tool like https://github.com/remarkjs/remark-validate-links to detect anything is moved or no longer valid.

Extra benefit comparing to pinning directly in source code:

numerology commented 4 years ago

@Bobgy That looks good to me. I can try to do item 2.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Bobgy commented 4 years ago

We've done this in above PRs.