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

Update config spec default values #7340

Merged
merged 3 commits into from
Aug 14, 2020
Merged

Update config spec default values #7340

merged 3 commits into from
Aug 14, 2020

Conversation

sarah-witt
Copy link
Contributor

@sarah-witt sarah-witt commented Aug 10, 2020

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

Copy link
Contributor

@kayayarai kayayarai left a comment

Choose a reason for hiding this comment

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

no docs to review

@@ -33,7 +33,7 @@ files:
value:
type: string
example: localhost
default: None
default: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need default here at all? Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it could be left out- since it defaults to an empty string, it's hard to show that here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@@ -42,7 +42,7 @@ files:
value:
type: string
example: datadog
default: None
default: ''
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@sarah-witt
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@@ -58,7 +58,7 @@ files:
value:
type: number
example: 3306
default: None
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
default: 0
default: null

Maybe here too? Or should default be the example since it's option is enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure!

@coignetp coignetp merged commit 9d030bd into master Aug 14, 2020
@coignetp coignetp deleted the sarah/update-conf-mysql branch August 14, 2020 13:20
coignetp pushed a commit that referenced this pull request Aug 14, 2020
* Make config spec accurate

* Remove default

* Update default port
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.

4 participants