-
Notifications
You must be signed in to change notification settings - Fork 440
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
chore(docs): Migrate link formats #2687
Conversation
website/docs/cdktf/develop-custom-constructs/publishing-and-distribution.mdx
Show resolved
Hide resolved
We also need to adjust the links in our link shortener right? |
@DanielMSchmidt I'm not sure what you're referring to! Might be something I haven't seen yet 😊 |
We link to some of these pages here: https://github.com/hashicorp/terraform-cdk/blob/main/cdk.tf/vercel.json (edit: They all redirect from cdk.tf to various destinations) |
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.
Looks good to be merged by the way 🎉
(besides the three broken links that are already broken currently and which we will fix after this 👍)
@ansgarm ah okay, I see! @BRKalow or @thiskevinwang can probably provide stronger feedback on this since they have more familiarity with it. |
That is something that is managed by us, I think updating the links that point to developer.hashicorp.com would be enough, but could also be done in a follow-up PR 😇 |
@ansgarm my account doesn't have the permissions to merge this PR, so feel free to do it when you're ready! |
8b15976
to
e004116
Compare
@ashleemboyer 👋 Happy to merge this, however the "Test Link Rewrites" is still failing (but as I read in Slack, that is okay?) – it also appears that this workflow is only intended to run on this PR to check this change. |
@ansgarm yep it's OK the test is failing! I've noted it in the PR description as well. I'll go ahead and remove the file as you suggested! Be right back. 😊 |
@ansgarm file removed! This should be ready to go on my end. 😎 |
Mergeddddd 💯 I kept the discussion items open for a follow-up PR to fix the broken links |
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
What
.github/workflows/check-legacy-links-format.yml
- This is a workflow that checks if links in markdown content are in the new format. It does this by running the link rewrite script fromdev-portal
, and then asserting that no "rewriteable" links were found. This workflow will be used on subsequent PRs to maintain the new link format..github/workflows/test-link-rewrites.yml
- This is a workflow that executes e2e tests of this PR's changes against themain
branch's deploy preview. The tests only run for branches nameddocs/amb.migrate-link-formats
, so future PRs will not have the tests run against them. This is a temporary workflow.Reviewing
With the two new workflows in place, this PR should only need a spot-check.
Testing
/terraform/cdktf/api-reference/java
page. This is happening because the page is very large and failed to load in the test run. We know this because thecontentHrefs
being compared in the test are opposite: one is totally empty and the other has more than 10,000 links. Since this page's MDX file is not changed by this PR, we can ignore this failing test.