Skip to content
This repository was archived by the owner on Jun 11, 2025. It is now read-only.

Fix/gcp helm agent#314

Merged
nxtcoder17 merged 2 commits into
mainfrom
fix/gcp-helm-agent
Apr 12, 2024
Merged

Fix/gcp helm agent#314
nxtcoder17 merged 2 commits into
mainfrom
fix/gcp-helm-agent

Conversation

@nxtcoder17
Copy link
Copy Markdown
Member

@nxtcoder17 nxtcoder17 commented Apr 12, 2024

@nxtcoder17 nxtcoder17 requested a review from karthik1729 as a code owner April 12, 2024 05:27
@nxtcoder17 nxtcoder17 merged commit 1bfe52b into main Apr 12, 2024
@nxtcoder17 nxtcoder17 deleted the fix/gcp-helm-agent branch April 12, 2024 05:28
@nxtcoder17 nxtcoder17 self-assigned this Apr 12, 2024
Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

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

Hey @nxtcoder17 - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟡 Testing: 1 issue found
  • 🟢 Complexity: all looks good
  • 🟢 Docstrings: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

Comment thread Taskfile.yml
Comment on lines +29 to +32
set -x
whereis mocki
mocki --package $package_path --interface $interface_name > mocks/$mock_module_path/$file_name
set +x
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (code_refinement): Consider wrapping debug commands with a condition

Using set -x and set +x for debugging is useful, but consider wrapping these with a condition to enable them only when needed, to avoid verbose output in production.

Suggested change
set -x
whereis mocki
mocki --package $package_path --interface $interface_name > mocks/$mock_module_path/$file_name
set +x
if [ "$DEBUG" = "true" ]; then set -x; fi
whereis mocki
mocki --package $package_path --interface $interface_name > mocks/$mock_module_path/$file_name
if [ "$DEBUG" = "true" ]; then set +x; fi

name: "test 1: normal interface",
args: args{
packagePath: "kloudlite.io/cmd/mocki/internal/parser/test_data",
packagePath: "github.com/kloudlite/api/cmd/mocki/internal/parser/test_data",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

suggestion (testing): Consider adding tests for new package paths.

The change in package paths might affect how interfaces are parsed. It would be beneficial to add tests that specifically cover the resolution of interfaces and types in the new package structure to ensure that the parser behaves as expected.

Suggested change
packagePath: "github.com/kloudlite/api/cmd/mocki/internal/parser/test_data",
{
name: "test 8: interface resolution in new package structure",
args: args{
packagePath: "github.com/kloudlite/api/cmd/mocki/internal/parser/test_data",
interfaceName: "TypeResolutionTest",
},
wantErr: false,
},

abdheshnayak pushed a commit that referenced this pull request Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[KLO-274] Build Trigger should use already authenticated credentials (that is already used, when setting it up)

1 participant