Skip to content
This repository was archived by the owner on May 8, 2024. It is now read-only.

Ported simplified logic from #129 #131

Closed
wants to merge 2 commits into from

Conversation

edigaryev
Copy link
Contributor

The tests currently fail because of #130 (comment).

@edigaryev edigaryev requested a review from fkorotkov April 19, 2021 22:00
@fkorotkov
Copy link
Contributor

I'm concerned about the failures in only clone and only clone (precreated). IMO the precreated directory should not override CIRRUS_WORKING_DIR. What do you think?

@edigaryev
Copy link
Contributor Author

edigaryev commented Apr 20, 2021

I'm concerned about the failures in only clone and only clone (precreated). IMO the precreated directory should not override CIRRUS_WORKING_DIR. What do you think?

I don't get which override you're talking about? In these tests only CIRRUS_CLONE_DIR is defined by the user.

@fkorotkov
Copy link
Contributor

I don't get which override you're talking about? In these tests only CIRRUS_CLONE_DIR is defined by the user.

Yes, and with your logic CIRRUS_WORKING_DIR is set to the precreated directory. IMO CIRRUS_WORKING_DIR should be set to $CIRRUS_CLONE_DIR. 🤔

@edigaryev
Copy link
Contributor Author

I don't get which override you're talking about? In these tests only CIRRUS_CLONE_DIR is defined by the user.

Yes, and with your logic CIRRUS_WORKING_DIR is set to the precreated directory. IMO CIRRUS_WORKING_DIR should be set to $CIRRUS_CLONE_DIR. 🤔

But why'd anyone do this instead of only setting CIRRUS_WORKING_DIR correctly in the first place?

And in case someone uses this pattern, they get an empty working directory that can be used to build some artifacts from CIRRUS_CLONE_DIR.

@fkorotkov
Copy link
Contributor

But why'd anyone do this instead of only setting CIRRUS_WORKING_DIR correctly in the first place?

Because people can 😅 Let's think about how CIRRUS_CLONE_DIR and CIRRUS_WORKING_DIR will be described in the documentation and how to explain the defaults. With my changes it will look like #847. In your case I don't feel it's clear.

@edigaryev edigaryev closed this Apr 20, 2021
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.

2 participants