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.

Sign up for GitHub

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

Jump to bottom

Impression events for ucp #243

Merged
merged 10 commits into from Apr 6, 2023
Merged

Impression events for ucp #243

merged 10 commits into from Apr 6, 2023

Conversation

ivanklee86
Copy link
Collaborator

@ivanklee86 ivanklee86 commented Apr 4, 2023 •
edited

Description

  • Add impression events support.
    • Use blinker as an example implementation.
  • Add devcontainer for local/Codespace development.
  • Support Python 3.11 in tests.
  • Remove Slacks for PRs so forks don't fail.

Fixes #238

Type of change

Please delete options that are not relevant.

  • Bug fix (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)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • Unit tests
  • Spec Tests
  • Integration tests / Manual Tests

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@ivanklee86 ivanklee86 changed the title Finish implementing event callback, linting. Impression events for ucp Apr 4, 2023
@coveralls
Copy link

coveralls commented Apr 5, 2023 •
edited

Coverage Status

Coverage: 96.647% (+0.1%) from 96.5% when pulling 21323b8 on il/event_source into 5bbdc6c on main.

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
UnleashClient/api/features.py Outdated
@@ -61,8 +61,7 @@ def get_feature_toggles(url: str,
if resp.status_code not in [200, 304]:
log_resp_info(resp)
LOGGER.warning("Unleash Client feature fetch failed due to unexpected HTTP status code.")
# pylint: disable=broad-exception-raised
raise Exception("Unleash Client feature fetch failed!")
raise Exception("Unleash Client feature fetch failed!") # pylint: disable=broad-exception-raised
Copy link

Choose a reason for hiding this comment

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

E261: at least two spaces before inline comment

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
.devcontainer/post_install.sh Outdated

# Install package
sudo python -m venv .venv
source .venv/bin/activate
Copy link

Choose a reason for hiding this comment

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

22% of developers fix this issue

SC1091: Not following: .venv/bin/activate was not specified as input (see shellcheck -x).

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sonatype-lift ignore

Copy link

Choose a reason for hiding this comment

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

I've recorded this as ignored for this pull request.
If you change your mind, just comment @sonatype-lift unignore.

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
tests/unit_tests/test_client.py
@@ -574,3 +577,48 @@ def test_multiple_instances_are_unique_on_api_key(caplog):
UnleashClient(URL, "some-probably-unique-app-name", custom_headers={"Authorization": "penguins"})
UnleashClient(URL, "some-probably-unique-app-name", custom_headers={"Authorization": "hamsters"})
assert not all(["Multiple instances has been disabled" in r.msg for r in caplog.records])

@responses.activate
Copy link

Choose a reason for hiding this comment

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

E302: expected 2 blank lines, found 1

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
tests/unit_tests/test_client.py Outdated

if kw['data'].event_type == UnleashEventType.FEATURE_FLAG:
assert kw['data'].feature_name == 'testFlag'
assert kw['data'].enabled == True
Copy link

Choose a reason for hiding this comment

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

E712: comparison to True should be 'if cond is True:' or 'if cond:'

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
tests/unit_tests/test_client.py Outdated
assert kw['data'].enabled == True
elif kw['data'].event_type == UnleashEventType.VARIANT:
assert kw['data'].feature_name == 'testVariations'
assert kw['data'].enabled == True
Copy link

Choose a reason for hiding this comment

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

E712: comparison to True should be 'if cond is True:' or 'if cond:'

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
tests/unit_tests/test_client.py Outdated
assert kw['data'].feature_name == 'testVariations'
assert kw['data'].enabled == True
assert kw['data'].variant == 'VarA'

Copy link

Choose a reason for hiding this comment

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

W293: blank line contains whitespace

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

sonatype-lift[bot]
sonatype-lift bot reviewed Apr 5, 2023
View reviewed changes
tests/unit_tests/test_client.py Outdated
assert kw['data'].variant == 'VarA'

raise Exception("Random!")

Copy link

Choose a reason for hiding this comment

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

W293: blank line contains whitespace

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage
@sonatype-lift ignore Leave out the above finding from this PR
@sonatype-lift ignoreall Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
Click here to add LiftBot to another repo.

Help us improve LIFT! (Sonatype LiftBot external survey)

Was this a good recommendation for you? Answering this survey will not impact your Lift settings.

[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

@ivanklee86 ivanklee86 marked this pull request as ready for review April 5, 2023 04:46
@sighphyre sighphyre requested a review from daveleek April 5, 2023 06:50
daveleek
daveleek approved these changes Apr 5, 2023
View reviewed changes
Copy link

@daveleek daveleek left a comment

Choose a reason for hiding this comment

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

the impression data/events implementation looks good! I'll leave the rest up to @sighphyre!

sighphyre
sighphyre approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@sighphyre sighphyre left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@ivanklee86 @sighphyre
Update .devcontainer/devcontainer.json
21323b8 
Co-authored-by: Simon Hornby <liquidwicked64@gmail.com>
@ivanklee86 ivanklee86 merged commit 1fcedbf into main Apr 6, 2023
34 checks passed
@ivanklee86 ivanklee86 deleted the il/event_source branch April 6, 2023 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sonatype-lift sonatype-lift[bot] sonatype-lift left review comments

@daveleek daveleek daveleek approved these changes

@sighphyre sighphyre sighphyre approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Issues and PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Impression Events
4 participants
@ivanklee86 @coveralls @daveleek @sighphyre