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

Add support for diagnosticSettings #4363

Merged
merged 8 commits into from
Nov 1, 2024

Conversation

nishant221
Copy link
Contributor

@nishant221 nishant221 commented Oct 22, 2024

Closes #4248

What this PR does / why we need it:
Add support for a new resource DiagnosticSetting

Special notes for your reviewer:

How does this PR make you feel:
Excited :)

If applicable:

  • this PR contains documentation
  • this PR contains tests
  • this PR contains YAML Samples

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

This is really solid, good job. I made a few comments to improve your test, and you need to make a recording of your sample, but other than that it's very nearly complete.

Taskfile.yml Outdated Show resolved Hide resolved
docs/hugo/content/reference/_index.md Outdated Show resolved Hide resolved
v2/azure-arm.yaml Outdated Show resolved Hide resolved
@theunrepentantgeek
Copy link
Member

/ok-to-test sha=e7174dd

1 similar comment
@theunrepentantgeek
Copy link
Member

/ok-to-test sha=e7174dd

@nishant221 nishant221 force-pushed the feature/diagnosticsetting branch from 66e5c22 to 28a97cf Compare October 24, 2024 09:08
@theunrepentantgeek
Copy link
Member

CI is failing because you're missing an update to one file - v2/api/insights/versions_matrix.md.

Once that's included, we can run the full test suite and look at merging this contribution. Thanks!

Spoiler alert: I ran the full test suite locally, and it passed. 😉

@nishant221
Copy link
Contributor Author

@theunrepentantgeek Thanks for the valuable feedbacks. Added the missing updates to the file - v2/api/insights/versions_matrix.md . Kindly have a re-look.
Thanks

@theunrepentantgeek
Copy link
Member

/ok-to-test sha=2be47b4

@matthchr
Copy link
Member

/ok-to-test sha=27a974c

@theunrepentantgeek
Copy link
Member

A couple of markdown files are generating with different content, looks like the generator is now picking up on the sample and is including links in the index files. I've confirmed this locally:

$ git status
On branch feature/diagnosticsetting
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   docs/hugo/content/reference/_index.md
        modified:   docs/hugo/content/reference/insights/_index.md

Run task controller:generate-types to regenerate them. Once CI is clean, we'll merge this.

@nishant221
Copy link
Contributor Author

Ran the above mentioned command and committed the changes 0f45678

@matthchr
Copy link
Member

matthchr commented Nov 1, 2024

/ok-to-test sha=0f45678

@matthchr matthchr added this pull request to the merge queue Nov 1, 2024
Merged via the queue into Azure:main with commit b4ea117 Nov 1, 2024
8 checks passed
@theunrepentantgeek
Copy link
Member

Thanks @nishant221, great contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants