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: Security must be a local file, not from repository #100

Merged

Conversation

c-pius
Copy link
Contributor

@c-pius c-pius commented Nov 20, 2024

Description

Changes proposed in this pull request:

  • security config read from local file, not from git
  • extended e2e tests to ensure multiple protecode entries are considered

Related issue(s)

@c-pius c-pius requested review from a team as code owners November 20, 2024 14:54
@c-pius c-pius requested a review from medmes November 20, 2024 16:17
@c-pius c-pius force-pushed the feat/security-config-must-be-local-file branch from 3e04755 to bff7dae Compare November 21, 2024 12:41
Copy link
Member

@medmes medmes left a comment

Choose a reason for hiding this comment

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

It looks good for me, the only thing is I have a concern about the create Service:

internal/service/create/create.go:65

It handles a lot of responsibilities, module configuration, git sources, security configuration, component archives, registry operations, module templates, CRD parsing, and file resolution.

But I think, we can work on it in a separate issue. What do you thinnk?

@medmes medmes self-requested a review November 21, 2024 14:43
@ruanxin ruanxin enabled auto-merge (squash) November 21, 2024 14:50
@c-pius
Copy link
Contributor Author

c-pius commented Nov 21, 2024

It handles a lot of responsibilities, module configuration, git sources, security configuration, component archives, registry operations, module templates, CRD parsing, and file resolution.

But I think, we can work on it in a separate issue. What do you thinnk?

Thanks! I have some doubts that there is a significant improvement possible. Eventually, The Run of create.go is a orchestration function that just composes the overall flow of functionality and delegates execution to the specific services. But if you have an idea how we can improve this we can try :)

@medmes
Copy link
Member

medmes commented Nov 21, 2024

Yes,As we discussed maybe we can think about another naming if It's an orchestrator and not a in a sense of a service. I will rethink about potential naming to this encapsulating struct. Thanks Christoph!

@ruanxin ruanxin merged commit 1230ee5 into kyma-project:main Nov 21, 2024
10 checks passed
@c-pius c-pius deleted the feat/security-config-must-be-local-file branch November 22, 2024 07:03
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.

[Module Catalog] Add module images as additional Resources from sec-scanners-config.yaml
5 participants