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

add CertManager function for listing certificates of any kind #5334

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mitjaziv
Copy link
Contributor

@mitjaziv mitjaziv commented Jul 19, 2023

Description

Change adds ListAny function to CertManager, which enables listing of certificates of
any kind. This change would enable, decoupling of Tyk Identity Broker from Tyk Gateway.

Related Issue

TykTechnologies/tyk-identity-broker#320

Motivation and Context

This change will enable to decoupling of Tyk Gateway from Tyk Identity Broker, by using
internal Tyk Identity Broker CertManager interface.

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 19, 2023

API tests result: success
Branch used: refs/pull/5334/merge
Commit:
Triggered by: pull_request (@mitjaziv)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 19, 2023

API tests result: success
Branch used: refs/pull/5334/merge
Commit: 87eec13
Triggered by: pull_request (@mitjaziv)
Execution page

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Jul 19, 2023

API tests result: success
Branch used: refs/pull/5334/merge
Commit: 8b66da1
Triggered by: pull_request (@tbuchaillot)
Execution page

@buger
Copy link
Member

buger commented Aug 2, 2023

@CodiumAI-Agent review

@CodiumAI-Agent
Copy link

PR Analysis

  • 🎯 Main theme: Adding a new function to list certificates of any kind
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: Yes
  • Focused PR: Yes, the PR is focused as it adds a new function to the CertManager and includes relevant tests for the same. The title and description clearly explain the purpose of the PR.
  • 🔒 Security concerns: No, the PR does not introduce any obvious security concerns. It is focused on listing certificates, and does not appear to introduce any vulnerabilities such as SQL injection, XSS, CSRF, etc.

PR Feedback

  • 💡 General PR suggestions: The PR is well-structured and includes relevant tests for the new function. However, it would be beneficial to include comments in the test cases explaining what each test is doing. Additionally, the PR could benefit from a more detailed description of how the new function contributes to the decoupling of Tyk Gateway from Tyk Identity Broker.

How to use

Tag me in a comment '@CodiumAI-Agent' and add one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve: Suggest improvements to the code in the PR.
/ask <QUESTION>: Pose a question about the PR.

To edit any configuration parameter from 'configuration.toml', add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Aug 2, 2023

API tests result: success
Branch used: refs/pull/5334/merge
Commit: a8908de
Triggered by: pull_request (@buger)
Execution page

Change adds ListAny function to CertManager, which enables listing
of certificates of any kind. This change would enable, decoupling of
Tyk Identity Broker from Tyk Gateway.
@titpetric titpetric force-pushed the add/certmanager-listany branch from a8908de to 9aad2f4 Compare August 24, 2023 14:21
@github-actions
Copy link
Contributor

API Changes

no api changes detected

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

@Tyk-ITS
Copy link
Contributor

Tyk-ITS commented Aug 24, 2023

API tests result: success
Branch used: refs/pull/5334/merge
Commit: 9aad2f4
Triggered by: pull_request (@titpetric)
Execution page

@tbuchaillot tbuchaillot removed their request for review March 5, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants