Skip to content

Conversation

EwanValentine
Copy link
Contributor

@EwanValentine EwanValentine commented May 25, 2021

Description

Description of what this PR does. What have you added or changed, and why? If it fixes a bug or resolves a feature request, be sure to link to that issue.

  • ADDED boto3 timeout/max retries as env vars
  • FIXED minor error handling issue for 'none found' scenario
  • ADDED example file for testing

Review Checks

Please check if the PR fulfills these requirements:

Put an x in the boxes that apply, Remove any lines that do not apply

  • 📝 The commit message is clear and descriptive
  • 🔐 The Security Considerations section in the PR description is complete - Please do not remove this
  • ✅ Tests for the changes have been added and run successfully including the new changes - no tests (yet)
  • 📄 Documentation has been added / updated (for bug fixes / features)

Dependencies

Add links to any pull requests or documentation related to this pull request.

YOUR LINK HERE

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Yes

Security Considerations

Are there any other security or data concerns to be aware of?

Please discuss the security implications/considerations relevant to the proposed change.
This may include...

  • security-relevant design decisions
  • concerns
  • important discussions
  • implementation-specific guidance and pitfalls
  • an outline of threats and risks and how they are being addressed.

No specific security concerns, minor change to existing API.

Types of change

What kind of change does this Pull Request introduce?
Put an x in the boxes that apply

  • 🐛 Bugfix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 🛠 Adding or updating configuration files, development scripts etc.
  • ♻️ Refactoring (no functional changes, no api changes)
  • 🧹 Chore (removing redunant files, fixing typos etc.)
  • 📄 Documentation Update
  • ❓ Other (if none of the other choices apply)

If "Other" please specify

YOUR TEXT HERE

Testing

Please include steps that the reviewer can follow in order to test the changes

  1. Update the 'example.py' file locally with a real service/instance.
  2. Run $ python example.py, ensure it still works as expected.
  3. Run $ python example.py again, but with an invalid location, ensure it's handled gracefully.
  4. Run $ BOTO_READ_TIMEOUT=0.001 BOTO_MAX_ATTEMPTS=1 python examples.py - should timeout immediately.

Other information

Please add any other information that would be useful for the reviewer.

@EwanValentine EwanValentine requested a review from azhar22k May 25, 2021 15:40
Copy link
Member

@azhar22k azhar22k left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Added some questions
Also, you might want to release (Github Release) after updating RC compatible version here which will trigger the github action to test it out

@EwanValentine
Copy link
Contributor Author

@azhar22k I've updated the version number etc, did we want to merge this then create the Github release, or create a release candidate from this branch first?

@EwanValentine EwanValentine requested a review from azhar22k May 26, 2021 16:20
@azhar22k
Copy link
Member

@azhar22k I've updated the version number etc, did we want to merge this then create the Github release, or create a release candidate from this branch first?

FIne either way. I would say first do an alpha release (will need to update version with match python semver first) from this branch, test it out, then make a major release from master.

But it is a small change and you would have already tested locally so not much issue I think 😁

@EwanValentine
Copy link
Contributor Author

@azhar22k there we go! https://github.com/peak-ai/ais-service-discovery-python/releases/tag/v0.2.1-rc1

Co-authored-by: Vishnu Agarwal <vishnu.agarwal@peak.ai>
Copy link

@knowvishnu knowvishnu left a comment

Choose a reason for hiding this comment

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

few comments

Ewan Valentine and others added 4 commits July 21, 2021 09:50
Co-authored-by: Vishnu Agarwal <vishnu.agarwal@peak.ai>
Co-authored-by: Vishnu Agarwal <vishnu.agarwal@peak.ai>
Co-authored-by: Vishnu Agarwal <vishnu.agarwal@peak.ai>
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.

3 participants