Skip to content
This repository has been archived by the owner on Oct 11, 2024. It is now read-only.

remove DiscreteScopes() from selectors #2036

Merged
merged 4 commits into from
Jan 10, 2023
Merged

remove DiscreteScopes() from selectors #2036

merged 4 commits into from
Jan 10, 2023

Conversation

ryanfkeepers
Copy link
Contributor

Description

DiscreteScopes is a vestigial func from when scopes contained the list of resource owners to track. That behavior is no longer in use.

Does this PR need a docs update or release note?

  • ⛔ No

Type of change

  • 🧹 Tech Debt/Cleanup

Issue(s)

Test Plan

  • ⚡ Unit test

@ryanfkeepers ryanfkeepers added the tech-debt Non-feature, non-bug improvements to the codebase. label Jan 5, 2023
@ryanfkeepers ryanfkeepers requested a review from ashmrtn January 5, 2023 00:38
@ryanfkeepers ryanfkeepers self-assigned this Jan 5, 2023
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 5, 2023 00:38 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 5, 2023 00:38 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 5, 2023 00:38 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 5, 2023 00:44 — with GitHub Actions Inactive
Base automatically changed from issue-1617-6 to main January 9, 2023 22:42
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 9, 2023 23:28 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 9, 2023 23:28 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 9, 2023 23:28 — with GitHub Actions Inactive
ashmrtn
ashmrtn approved these changes Jan 10, 2023
View reviewed changes
src/internal/operations/backup.go Outdated
// TODO: should always be 1, since backups are 1:1 with resourceOwners now.
opStats.resourceCount = len(data.ResourceOwnerSet(cs))
// should always be 1, since backups are 1:1 with resourceOwners.
opStats.resourceCount++
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is there ever a time we'll need to call BackupOp.Run() multiple times and have resourceCount > 1? Otherwise it seems easier to grok if it's just opStats.resourceCount = 1 since readers won't need to know the previous value of opStats.resourceCount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way works for now. We should probably clean up that tracking to match current behavior in general.

@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:09 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:09 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:09 — with GitHub Actions Inactive
@aviator-app
Copy link
Contributor

aviator-app bot commented Jan 10, 2023 •
edited
Loading

Aviator status

Aviator will automatically update this comment as the status of the PR changes.

This PR was merged using Aviator.

@aviator-app aviator-app bot temporarily deployed to Testing January 10, 2023 21:31 Inactive
@aviator-app aviator-app bot temporarily deployed to Testing January 10, 2023 21:31 Inactive
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Jan 10, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented Jan 10, 2023

PR failed to merge with reason: some CI status(es) failed.
Failed CI(s): Website-Linting

@aviator-app aviator-app bot removed the blocked Upstream item prevents completion label Jan 10, 2023
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:57 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:57 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:57 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:58 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 21:58 — with GitHub Actions Inactive
@aviator-app aviator-app bot added the blocked Upstream item prevents completion label Jan 10, 2023
@aviator-app
Copy link
Contributor

aviator-app bot commented Jan 10, 2023

PR failed to merge with reason: some CI status(es) failed.
Failed CI(s): Test-Suite-Trusted

@aviator-app aviator-app bot removed the blocked Upstream item prevents completion label Jan 10, 2023
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:55 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:55 — with GitHub Actions Inactive
@ryanfkeepers
remove DiscreteScopes() from selectors
b366529 
DiscreteScopes is a vestigial func from when scopes
contained the list of resource owners to track.  That
behavior is no longer in use.
@ryanfkeepers
set resource count to 1
b25df2c
@ryanfkeepers
fix reappearance of discreteScopes
e17d9e8
@ryanfkeepers
fix resource count
a8f008d
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:55 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:55 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:55 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:56 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:56 — with GitHub Actions Inactive
@ryanfkeepers ryanfkeepers temporarily deployed to Testing January 10, 2023 22:56 — with GitHub Actions Inactive
@aviator-app aviator-app bot merged commit 580934b into main Jan 10, 2023
@aviator-app aviator-app bot deleted the issue-1617-7 branch January 10, 2023 23:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashmrtn ashmrtn ashmrtn approved these changes

Assignees

@ryanfkeepers ryanfkeepers

Labels
mergequeue tech-debt Non-feature, non-bug improvements to the codebase.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ryanfkeepers @ashmrtn