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: verify if targetBranch is present in git repo #511

Merged
merged 3 commits into from
Apr 13, 2023
Merged

feat: verify if targetBranch is present in git repo #511

merged 3 commits into from
Apr 13, 2023

Conversation

fty4
Copy link
Contributor

@fty4 fty4 commented Jan 6, 2023

What this PR does / why we need it:

If a targetBranch is not present in the git repository the ct list-changed command will fail. The error message for this is not really meaningful.

This PR will validate if the branch exists and throw a error message which the user will understand.

Because the default branch in GitHub changed from master to main this is a common problem users are facing.

I've built the chart-testing and tested it with master branch as follows:

$ ct list-changed
Error: targetBranch 'master' does not exist
targetBranch 'master' does not exist

Which issue this PR fixes:

closes #330

Special notes for your reviewer:

If a targetBranch is not present in the git repository the `ct list-changed` command will fail.
The error message for this is not really meaningful.

This commit will validate if the branch exists and throw a error message which the user will understand.

closes #330

Signed-off-by: Marco Lecheler <[email protected]>
@davidkarlsen
Copy link
Member

if you look in chart_test.go there is a mock, if fails on the missing method now. Maybe it is possible to add a test there. At least the method needs to be stubbed out.

@fty4
Copy link
Contributor Author

fty4 commented Jan 9, 2023

@davidkarlsen thank you for your comment.

I've now mocked the test for func chart.BranchExists().
I guess at this moment it would not be that easy to add a real test.
Also I was wondering where to put this test - there is no order in the file, right?

The linter also suggested to change the error msg generation.

Note: required force-push because of missing DCO

@cpanato cpanato requested a review from davidkarlsen March 24, 2023 14:11
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add better error message if "target branch" is missing - exit status 128
3 participants