Skip to content

Make cloud events config type lowercase#6408

Merged
davidmirror-ops merged 1 commit into
flyteorg:masterfrom
Sovietaced:issue-6402
Apr 10, 2025
Merged

Make cloud events config type lowercase#6408
davidmirror-ops merged 1 commit into
flyteorg:masterfrom
Sovietaced:issue-6402

Conversation

@Sovietaced
Copy link
Copy Markdown
Member

@Sovietaced Sovietaced commented Apr 9, 2025 •
edited
Loading

Tracking issue

Closes: #6402

Why are the changes needed?

Normalizes cloud event type configuration to lowercase to avoid misconfiguration and match the documentation (specifically regarding Kafka).

What changes were proposed in this pull request?

How was this patch tested?

Existing unit tests.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR normalizes cloud event configuration types to lowercase to prevent misconfigurations and align with documentation standards. Changes include adding the 'strings' package import, modifying switch logic for lowercase conversion, and updating the Kafka receiver constant to match the new standard.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

@flyte-bot
Copy link
Copy Markdown
Collaborator

flyte-bot commented Apr 9, 2025 •
edited
Loading

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - Bito Automatic Review Skipped - Draft PR

    Bito didn't auto-review because this pull request is in draft status.
    To trigger review, mark the PR as ready or type /review in the comment and save.
    You can change draft PR review settings here, or contact the agent instance creator at [email protected].

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.48%. Comparing base (f2a1ad7) to head (5753429).
Report is 2 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #6408      +/-   ##
==========================================
- Coverage   58.49%   58.48%   -0.01%     
==========================================
  Files         940      940              
  Lines       71554    71554              
==========================================
- Hits        41853    41848       -5     
- Misses      26520    26525       +5     
  Partials     3181     3181
Flag Coverage Δ
unittests-datacatalog 59.03% <ø> (ø)
unittests-flyteadmin 56.25% <100.00%> (-0.03%) ⬇️
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 64.70% <ø> (ø)
unittests-flyteidl 76.12% <ø> (ø)
unittests-flyteplugins 60.94% <ø> (ø)
unittests-flytepropeller 54.79% <ø> (ø)
unittests-flytestdlib 64.04% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

davidmirror-ops
davidmirror-ops reviewed Apr 9, 2025
View reviewed changes
Comment thread flyteadmin/pkg/async/cloudevent/factory.go

var sender interfaces.Sender
switch cloudEventsConfig.Type {
switch strings.ToLower(cloudEventsConfig.Type) {
Copy link
Copy Markdown
Contributor

@davidmirror-ops davidmirror-ops Apr 9, 2025

Choose a reason for hiding this comment

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

nice, thanks for making it backwards compatible

@Sovietaced Sovietaced added the fixed For any bug fixes label Apr 9, 2025
@Sovietaced Sovietaced marked this pull request as ready for review April 9, 2025 17:44
@flyte-bot
Copy link
Copy Markdown
Collaborator

flyte-bot commented Apr 9, 2025 •
edited
Loading

Code Review Agent Run #335773

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 5753429..5753429
    • flyteadmin/pkg/async/cloudevent/factory.go
    • flyteadmin/pkg/async/cloudevent/implementations/sender.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Copy Markdown
Collaborator

flyte-bot commented Apr 9, 2025

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Normalized Cloud Events Configuration

- factory.go - Added the 'strings' package import and modified the switch statement to use strings.ToLower for normalizing the cloud event type.

- sender.go - Updated the Kafka Receiver constant from 'Kafka' to 'kafka' to ensure consistent lowercase configuration.

@davidmirror-ops davidmirror-ops merged commit fad6b77 into flyteorg:master Apr 10, 2025
49 of 50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidmirror-ops davidmirror-ops davidmirror-ops approved these changes

Assignees

No one assigned

Labels

fixed For any bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Configuring cloudEvents with Kafka doesn't produce events nor errors

3 participants

@Sovietaced @flyte-bot @davidmirror-ops