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

Restructure test directories and improve temp dir error handling #59

Merged
merged 2 commits into from
Feb 11, 2021

Conversation

mmulholla
Copy link
Contributor

@mmulholla mmulholla commented Feb 10, 2021

  1. Restructured the directories.
  2. Restructure required a minor change to the tmp directory processing, so I added extra error checking to temp directory processing based on previous comments. Basically now if the temp directory can't be created the current directory will be used as temp. Also if the log file fails to be created the log function will print to the console instead.
  3. Note CreateTempDir is external - changes coming require that

// if cannot create subdirectory just use the base tmp directory
tempDir = tmpDir
}
} else if err != nil {
Copy link
Collaborator

@yangcao77 yangcao77 Feb 10, 2021

Choose a reason for hiding this comment

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

why we need this extra else statement?
can pre-define the error

var err error
if _, err = os.Stat(tempDir); os.IsNotExist(err) {
	err = os.Mkdir(tempDir, 0755)
}
if err != nil {
	LogErrorMessage(fmt.Sprintf("Failed to create temp directory %s will use %s : %v", tempDir, tmpDir, err))
	// if cannot create subdirectory just use the base tmp directory
	tempDir = tmpDi
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with your example if os.IsNotExist(err) returned true your second if would also return true which would be a bug. The else statement deals with errors which are not the os.IsNotExist(err) for example a permission error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The second won't return true if os.Mkdir passes, err will be nil in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah you removed my logic processing the os.Mkdir error, see where you are coming from now. Let me update it.

Copy link
Contributor Author

@mmulholla mmulholla Feb 10, 2021

Choose a reason for hiding this comment

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

Updated

@yangcao77
Copy link
Collaborator

change looks good to me

@yangcao77 yangcao77 added the lgtm label Feb 11, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maysunfaisal, mmulholla

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mmulholla mmulholla merged commit 951fc2a into devfile:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants