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: user suppressions adaptations for namespaces #2604

Merged
merged 4 commits into from
Nov 1, 2022

Conversation

Sidddddarth
Copy link
Member

Description

server can poll new namespaces endpoint to fetch the suppression for workspaces that constitute a namespace.

Notion Ticket

suppress-user adaptations for multitenant

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 43.75% // Head: 43.81% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (2391c2e) compared to base (078bf76).
Patch coverage: 90.69% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2604      +/-   ##
==========================================
+ Coverage   43.75%   43.81%   +0.06%     
==========================================
  Files         186      186              
  Lines       39893    39905      +12     
==========================================
+ Hits        17455    17485      +30     
+ Misses      21344    21324      -20     
- Partials     1094     1096       +2     
Impacted Files Coverage Δ
enterprise/suppress-user/noop.go 0.00% <0.00%> (ø)
enterprise/suppress-user/setup.go 0.00% <0.00%> (ø)
enterprise/suppress-user/suppressUser.go 85.95% <95.00%> (+4.85%) ⬆️
gateway/gateway.go 55.76% <100.00%> (ø)
utils/httputil/server.go 80.76% <0.00%> (-11.54%) ⬇️
router/router.go 74.82% <0.00%> (-0.18%) ⬇️
processor/processor.go 72.05% <0.00%> (+0.82%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Sidddddarth Sidddddarth force-pushed the feat.userSuppressionsAdaptations branch 4 times, most recently from 7f78c6c to 23c1eda Compare October 31, 2022 14:36
@atzoum atzoum force-pushed the feat.userSuppressionsAdaptations branch 2 times, most recently from c4150a7 to 941e1e3 Compare November 1, 2022 07:08
@Sidddddarth Sidddddarth marked this pull request as ready for review November 1, 2022 07:19
@Sidddddarth Sidddddarth force-pushed the feat.userSuppressionsAdaptations branch from 9a9a1bb to d23ec68 Compare November 1, 2022 08:28
// Identifier abstracts how data-plane can be identified to the control-plane.
//
// Including both a unique identifier and the authentication method.
type Identifier interface {
ID() string
BasicAuth() (string, string)
Type() deployment.Type
Copy link
Member

Choose a reason for hiding this comment

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

Why not use reflection to determine the type?

It is quite an idiomatic way, the only downside is performance.

}

func (suppressUser *SuppressRegulationHandler) setup(ctx context.Context) {
suppressUser.RegulationBackendURL = configBackendURL
switch suppressUser.ID.Type() {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest this approach mainly for consistency. I also find it more idiomatic, but that is up for discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, it is considered to be a bad practice requiring from the consumer of an exported type to have to know both about the generic interface type and the concrete implementation types, since it beats the original purpose of the interface, that is abstracting from the underlying implementation(s).

Copy link
Member

@lvrach lvrach left a comment

Choose a reason for hiding this comment

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

LGTM, a non-blocking comment regarding type.

@sonarcloud
Copy link

sonarcloud bot commented Nov 1, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
16.9% 16.9% Duplication

@chandumlg chandumlg merged commit 5c26d1b into master Nov 1, 2022
@chandumlg chandumlg deleted the feat.userSuppressionsAdaptations branch November 1, 2022 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants