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

[Doc] Fix links in preload sample #2722

Merged
merged 2 commits into from
Dec 12, 2019

Conversation

numerology
Copy link

@numerology numerology commented Dec 11, 2019

Use a fixed version instead of master.


This change is Reviewable

@numerology
Copy link
Author

/assign @IronPan

@numerology
Copy link
Author

/assign @neuromage

@Bobgy
Copy link
Contributor

Bobgy commented Dec 12, 2019

Good idea!

One more thing to add, did you also add a step to update README in release playbook? I think it would be best if we can automate pinning to release commit.

@Bobgy Bobgy self-requested a review December 12, 2019 02:57
@numerology
Copy link
Author

Good idea!

One more thing to add, did you also add a step to update README in release playbook? I think it would be best if we can automate pinning to release commit.

I think what I would like to do instead, is to automatically check whether the links work in READMEs. Because the link also contains other parts in addition to commit SHA that might subject to change, like the paramterized tfx oss sample in this PR.

@Bobgy
Copy link
Contributor

Bobgy commented Dec 12, 2019

I agree the extra check of whether the link works is useful.
I'd like to clarify that automating pinning to release commit avoids stale links cases that's also a problem:
although we updated README, prebuilt samples still link to old README files

@numerology
Copy link
Author

I agree the extra check of whether the link works is useful.
I'd like to clarify that automating pinning to release commit avoids stale links cases that's also a problem:
although we updated README, prebuilt samples still link to old README files

Yes agree, but an out-dated one would be better than a broken one I guess? If MKP deployment gets upgraded on a regular basis, changes in README should be picked up in an acceptable cadence.

@Bobgy
Copy link
Contributor

Bobgy commented Dec 12, 2019

If it's not too much effort, I'd like both constraints satisfied. Otherwise, I agree stale is better than broken.

@numerology
Copy link
Author

So I guess the question here is, how can we prevent accidentally breaking the link in the preload samples and docs. And I think we have the following options:

  1. Pin them to a specific commit SHA. This will make the links stale eventually.
  2. Add a 'link check' tests as part of the presubmit test to block possible breakage.

I think I agree to your point that 1 is not satisfying. Does 2 LGTY? That being said, this PR itself is just a quick fix to an issue we discovered today. I'm happy to drive option 2 if we decide to go that way and after I'm done we can detach the links with specific commit SHAs.

@Bobgy
Copy link
Contributor

Bobgy commented Dec 12, 2019

/lgtm
for the quick fix.

The best process is harder than I thought, after some reflection, this is what I came up with:

  1. Pin them to a release commit SHA
  2. During each release, use a script to automate updating the SHA to new release SHA
  3. Run a tool like https://github.com/remarkjs/remark-validate-links to detect anything is moved or no longer valid.

What do you think?

@numerology
Copy link
Author

/lgtm
for the quick fix.

The best process is harder than I thought, after some reflection, this is what I came up with:

  1. Pin them to a release commit SHA
  2. During each release, use a script to automate updating the SHA to new release SHA
  3. Run a tool like https://github.com/remarkjs/remark-validate-links to detect anything is moved or no longer valid.

What do you think?

That SGTM. Let me update this with some commit SHA and update the playbook as well.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 12, 2019
@IronPan
Copy link
Member

IronPan commented Dec 12, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 889105f into kubeflow:master Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants