-
Notifications
You must be signed in to change notification settings - Fork 68
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
Add cdktf docs #218
Add cdktf docs #218
Conversation
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.
Hey @DanielMSchmidt 👋 Thank you for submitting this.
I'm leaving a code-level review of the GitHub Actions workflow changes as given, but I think we need to decide on our side how we would prefer to handle actual CDKTF documentation updates. It's a little scary leaving this to a (currently untestable) process that can prevent releases if there are failures.
.github/workflows/release.yml
Outdated
- name: Build | ||
run: | | ||
go build -o terraform-provider-null | ||
|
||
- name: Upload test artifacts | ||
uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2 | ||
with: | ||
name: terraform-provider-null | ||
path: terraform-provider-null | ||
retention-days: 1 |
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.
GitHub really makes it annoying to get just the name of the repository. ${{ github.event.repository.name }}
might work in this case. That said, since this binary is not needed elsewhere in the workflow, is there any reason to not include the build in the cdktf documentation job and skip the artifact handling?
.github/workflows/release.yml
Outdated
node-version: "18.x" | ||
|
||
- name: Install cdktf-registry-docs | ||
run: npm install -g [email protected] |
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.
This feels like it should be moved to a package.json
file, similar to how the Digital team manages website packages. This will enable the dependency to be managed by Dependabot for updates, etc.
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'm not sure about this yet. Once we got more repositories onboarded to the workflow I'd expect certain patterns to emerge. Once we are there we might move all of this into a reusable workflow where inlining the used version of the tool would make the most sense.
What kind of process would you feel comfortable with? We use a cronjob + PR creation process the AWS provider would that be better from your perspective? |
e9747b5
to
aded2af
Compare
Co-authored-by: Brian Flad <[email protected]>
aded2af
to
b363851
Compare
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 contributions. |
This adds cdktf docs and a process to update them during the release process.
The conversion only works for code blocks that pass
terraform validate
therefore I had to add enough information to the snippets so that they got valid, I hope that's okay for you.