Skip to content

Conversation

@Thingus
Copy link
Contributor

@Thingus Thingus commented May 8, 2024

Closes #6580

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

Passes the output of flowapi.permissions.query_to_scopes through a set() before returning

@cypress
Copy link

cypress bot commented May 8, 2024

Passing run #22487 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge branch 'master' into dedupe-recieved-scopes
Project: FlowAuth Commit: 5532acbc53
Status: Passed Duration: 00:46 💡
Started: May 13, 2024 3:20 PM Ended: May 13, 2024 3:21 PM

Review all test suite changes for PR #6581 ↗︎

raise BadQueryError
agg_unit = await get_agg_unit(query_dict)
return [f"{agg_unit}:{tl_query_name}:{query_name}" for query_name in query_list]
return list(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to be a list, or could we return a set instead? (I don't think it matters much either way, but if we return a set it's perhaps clearer that the scopes will be non-duplicated)

Copy link
Contributor Author

@Thingus Thingus May 9, 2024

Choose a reason for hiding this comment

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

It's compared to other lists when we check it against the requested scopes; now I say that, a set would be better there too, but that probably belongs in a different PR

@Thingus Thingus added the FlowAPI Issues related to the FlowKit API label May 9, 2024
Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

👍

@Thingus Thingus added the ready-to-merge Label indicating a PR is OK to automerge label May 10, 2024
@codecov
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (3c06eed) to head (5532acb).

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6581       +/-   ##
===========================================
- Coverage   92.31%   77.11%   -15.20%     
===========================================
  Files         268      268               
  Lines       10586    10586               
  Branches      855      855               
===========================================
- Hits         9772     8163     -1609     
- Misses        676     2169     +1493     
- Partials      138      254      +116     

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

@mergify mergify bot merged commit 7488c12 into master May 13, 2024
@mergify mergify bot deleted the dedupe-recieved-scopes branch May 13, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FlowAPI Issues related to the FlowKit API ready-to-merge Label indicating a PR is OK to automerge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Same time queries producing duplicate scopes

3 participants