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

Bring version checker improvements to 5.1.x #2240

Conversation

hjafarpour
Copy link
Contributor

Description

This PR brings the version checker improvement to 5.1.x. The improvements were initially merged into master but we want to include it in 5.1 release.
The original PR that was merged into master is #2020 .

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@hjafarpour hjafarpour requested a review from a team December 5, 2018 10:00
@apurvam
Copy link
Contributor

apurvam commented Dec 5, 2018

Thanks @hjafarpour.. Was this tested with the server to make sure that things continue to work?

@hjafarpour
Copy link
Contributor Author

@apurvam I tested it manually with the phone-home and it works fine. The data is received correctly. How to handle it will need some minor change in backend.

@apurvam
Copy link
Contributor

apurvam commented Dec 6, 2018

Why are there two commits? this should just cherry pick from master on to 5.1 and have exactly one commit.

@hjafarpour
Copy link
Contributor Author

The other PR had 27 commits so it was easier to just apply the changes directly. Also there are conflicting changes in master that are not in 5.1 which version checker relied on.

Copy link
Contributor

@vcrfxia vcrfxia left a comment

Choose a reason for hiding this comment

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

Thanks @hjafarpour ! LGTM.

@apurvam
Copy link
Contributor

apurvam commented Dec 7, 2018

The other PR had 27 commits so it was easier to just apply the changes directly. Also there are conflicting changes in master that are not in 5.1 which version checker relied on.

Not sure I follow. There is just one commit on master 610a518

Did you do git checkout 5.1.x && git cherry-pick 610a518b1fe8ac7d21d46095d4f203af1f8ddcad and then resolve conflicts?

Or was it something else?

I think we should only use git cherry pick for operations like this, and document any conflicts that had to be resolved in the description / commit message. Makes the review much easier.

* Added isActive field to the ksql support metrics.

* Added tests.

* Updated the active status detection interval to 24 hours.

* Applied review feedback.

* Some refactoring.

* Fix tests

* Added to standalone.

* Test fix

* Refactoring based on Andy's feedabck.

* Refactoring with Andy!

* Change frequency to hour.

* CHange the frequency back to 24 hours.

* More changes addressing feedback.

* Added a comment.

* Temp commit

* Applied the feedback.

* Applied more feedback

* more feedback!

* Final feedback from Andy!

* Feedback from Victoria.
@hjafarpour hjafarpour force-pushed the Bring-Version-Checker-Improvements-To-5-1 branch from 6f4a443 to f1e4fbc Compare December 7, 2018 07:22
@hjafarpour
Copy link
Contributor Author

@apurvam changed it to one commit using cherry pick as you suggested.

Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

LGTM!

@hjafarpour hjafarpour merged commit 4336b2f into confluentinc:5.1.x Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants