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

fix(inputs.sqlserver): More precise version check #12384

Merged
merged 1 commit into from
Jan 9, 2023
Merged

fix(inputs.sqlserver): More precise version check #12384

merged 1 commit into from
Jan 9, 2023

Conversation

spaghettidba
Copy link
Contributor

Some queries do not work with 2008R2 RTM, but they need at least SP2
Queries changed:

  • sqlServerProperties

  • sqlServerVolumeSpace

  • Updated associated README.md.

  • Wrote appropriate unit tests.

  • Pull request title or commits are in conventional commit format

Some queries do not work with 2008R2 RTM, but they need at least SP2
@telegraf-tiger telegraf-tiger bot added the fix pr to fix corresponding bug label Dec 13, 2022
@srebhan srebhan self-assigned this Dec 13, 2022
@srebhan
Copy link
Contributor

srebhan commented Dec 13, 2022

@Trovalo can you please take a look!?

@srebhan srebhan requested a review from Trovalo December 13, 2022 10:19
@srebhan srebhan changed the title fix: More precise version check for sqlserver fix(inputs.sqlserver): More precise version check Dec 13, 2022
@srebhan srebhan added area/sqlserver plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Dec 13, 2022
@spaghettidba spaghettidba closed this by deleting the head repository Dec 13, 2022
@Trovalo Trovalo reopened this Dec 13, 2022
@Trovalo
Copy link
Collaborator

Trovalo commented Dec 13, 2022

The PR is ok,
It will avoid execution errors on the oldest versions of SQL Server (supported by Telegraf).

@srebhan do we also need to create an issue or having just the PR is fine?
(not sure about what "Semantic PR and Commit Messages" means, is there a check over the PR text itself?)

@telegraf-tiger
Copy link
Contributor

@srebhan
Copy link
Contributor

srebhan commented Dec 16, 2022

@Trovalo I think the text in the PR is clear enough to not requiring an issue, what do you think @powersj?

Regarding the "Semantic PR..." error, it comes from the fact that this PR has only one commit, so the commit also needs to adhere to the semantic PR standards... We can fix this manually on this PR, but please keep this in mind for future PRs @spaghettidba!

@srebhan srebhan closed this Dec 16, 2022
@srebhan srebhan reopened this Dec 16, 2022
@srebhan
Copy link
Contributor

srebhan commented Dec 16, 2022

@spaghettidba can you please try to rebase this PR on the latest master to (hopefully) get CircleCI to retry?!?

@spaghettidba
Copy link
Contributor Author

Sorry, I have no idea how to do that. My GitHub abilities are extremely limited.
Could you please help me?

@Trovalo
Copy link
Collaborator

Trovalo commented Dec 16, 2022

Running git rebase master or git rebase master _YourbranchName_ should suffice... but since the original repo has been deleted that's not possible.
Honestly, I'm not sure about how to "fix" this... can we edit the commit message and force CircleCI to run again? or do we need a "clean" PR (meaning closing this one and replicating it in a new one)?

Copy link
Contributor

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks @spaghettidba for the fix! Given that this seems not to touch any go-code, I'm also find with just merging it as-is. @powersj what do you think?

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Dec 19, 2022
@srebhan srebhan removed their assignment Jan 9, 2023
@powersj powersj closed this Jan 9, 2023
@powersj powersj reopened this Jan 9, 2023
@powersj powersj merged commit d476018 into influxdata:master Jan 9, 2023
srebhan pushed a commit that referenced this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sqlserver fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants