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

Create a folder when it doesn't exist and is needed for the report #326

Merged
merged 2 commits into from
Feb 17, 2022

Conversation

iblancasa
Copy link
Collaborator

Signed-off-by: Israel Blancas iblancas@redhat.com

Fixes #325

@iblancasa
Copy link
Collaborator Author

One of the contributing steps is:

If your proposed change is accepted, and you haven't already done so, sign a Contributor License Agreement (see details above).

Where can I sign it? Thanks! ☺️

@kaiwalyajoshi
Copy link
Member

kaiwalyajoshi commented Oct 4, 2021

@iblancasa 👋
Many thanks for your contributions.

You'll need to sign your commits to comply with the DCO found here

Here's the document on how to sign commits for Github: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

Edit: Linked to DCO rather than the CNCF CLA.

Signed-off-by: Israel Blancas <iblancas@redhat.com>
@iblancasa
Copy link
Collaborator Author

iblancasa commented Oct 4, 2021

Hi @kaiwalyajoshi, it seems I fixed the problem signing the commits :) Thanks!

@iblancasa
Copy link
Collaborator Author

Is there anything else I can do to include this change? Thanks!

@kensipe
Copy link
Member

kensipe commented Feb 13, 2022

this has been hanging out there a while... apologies! looking

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

At a min, could you shorten the err var name. and consider if other errors are important to return (which will drive if it is reasonable to "reuse" the err var.

I'll be quick on re-review. Thanks for the contribution!

pkg/report/report.go Outdated Show resolved Hide resolved
pkg/report/report.go Outdated Show resolved Hide resolved
Signed-off-by: Israel Blancas <iblancas@redhat.com>
@iblancasa
Copy link
Collaborator Author

@kensipe thanks for your review!

Copy link
Member

@kensipe kensipe left a comment

Choose a reason for hiding this comment

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

/lgtm

I'm somewhat anti- elseif in go. what do you think of:

	_, err := os.Stat(dir)
	switch  {
	case os.IsNotExist(err):
		err = os.MkdirAll(dir, 0755)
		if err != nil {
			return err
		}
	case err != nil:
		return nil
	}

I'm good either way. Expecting that a day will give thinking / response time. Will merge in either state tomorrow.

@kensipe
Copy link
Member

kensipe commented Feb 17, 2022

and thanks for the contribution! for awareness... Looking to do a release end of this week with go 1.17 bump and this change.

@iblancasa
Copy link
Collaborator Author

@kensipe awesome! Thanks

@kensipe kensipe merged commit 15ae036 into kudobuilder:main Feb 17, 2022
@iblancasa iblancasa deleted the create-folder-report branch February 17, 2022 15:57
iblancasa added a commit to iblancasa/kuttl that referenced this pull request Nov 17, 2022
…udobuilder#326)

Signed-off-by: Israel Blancas <iblancas@redhat.com>
Signed-off-by: Israel Blancas <iblancasa@gmail.com>
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.

The test report will not be generated if the folder to save the report doesn't exist
3 participants