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

feat(workflow-engine): add LatestReleaseConditionHandler #83076

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

ameliahsu
Copy link
Member

add LatestReleaseConditionHandler

@ameliahsu ameliahsu requested review from cathteng and a team January 8, 2025 00:45
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #83076      +/-   ##
==========================================
+ Coverage   87.56%   87.58%   +0.01%     
==========================================
  Files        9446     9448       +2     
  Lines      536694   536810     +116     
  Branches    21118    21118              
==========================================
+ Hits       469976   470153     +177     
+ Misses      66359    66298      -61     
  Partials      359      359              

Copy link
Member

@cathteng cathteng left a comment

Choose a reason for hiding this comment

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

refactor nits

def get_latest_release(environment_id: int | None, event: GroupEvent) -> Release | None:
cache_key = get_project_release_cache_key(event.group.project_id, environment_id)
cached_latest_release = cache.get(cache_key)
if cached_latest_release is None:
Copy link
Member

Choose a reason for hiding this comment

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

nit: can early return the cached_latest_release if cached_latest_release is not None

def evaluate_value(job: WorkflowJob, comparison: Any) -> bool:
event = job["event"]
workflow = job.get("workflow")
environment_id = workflow.environment_id if workflow else None
Copy link
Member

Choose a reason for hiding this comment

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

you can grab the environment object itself via workflow.environment, that way you don't have to re-lookup the environment on line 25, and use environment.id when you need the id

line 25:

environments = [Environment.objects.get(id=environment_id)]

Comment on lines 34 to 41
latest_release = Release.objects.filter(
version=latest_release_version, organization_id=organization_id
).first()
if latest_release:
cache.set(cache_key, latest_release, 600)
return latest_release
else:
cache.set(cache_key, False, 600)
Copy link
Member

@cathteng cathteng Jan 8, 2025

Choose a reason for hiding this comment

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

we could slightly rewrite this? this is because first() either returns the object or None

Suggested change
latest_release = Release.objects.filter(
version=latest_release_version, organization_id=organization_id
).first()
if latest_release:
cache.set(cache_key, latest_release, 600)
return latest_release
else:
cache.set(cache_key, False, 600)
latest_release = Release.objects.filter(
version=latest_release_version, organization_id=organization_id
).first()
cache.set(cache_key, latest_release or False, 600)
return latest_release

@ameliahsu ameliahsu force-pushed the mia/aci/latest-release branch from 67ee08d to 2b19533 Compare January 9, 2025 00:27
@ameliahsu ameliahsu merged commit 398b0d2 into master Jan 9, 2025
49 checks passed
@ameliahsu ameliahsu deleted the mia/aci/latest-release branch January 9, 2025 17:37
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants