PR#267: standard-test-source: export source dir - standard-test-roles

standard-test-roles

#267 standard-test-source: export source dir

Closed 5 years ago by astepano. Opened 5 years ago by ssahani.

ssahani/standard-test-roles issues-263 into master

Make test souce unpacking configurable

Susant Sahani • 5 years ago

8198a21

roles/str-common-init/defaults/main.yml

file modified

+1 -1

		`@@ -2,7 +2,7 @@`

		`tests: []`
		`required_packages: []`
		`- tenv_workdir: /var/str/`
		`+ tenv_workdir: "{{ lookup('env', 'TENV_WORKDIR')\|default('/var/str', true) }}"`
		`remote_artifacts: /tmp/artifacts/`
		`test_runner_inventory_name: test-runner`
		`artifacts: "{{ lookup('env', 'TEST_ARTIFACTS')\|default('./artifacts', true) }}"`

ssahani commented 5 years ago

This works make it configurable to choose where to unpack
the source.

export TENV_WORKDIR='blah/blah/blah'

astepano commented 5 years ago

@ssahani

1. We do not want allow user to define workdir. Not now. Without real-test case that asks for it.
2. #263 - ask to export of workdir with help of env variable to Ansible tasks. #263 - it doesn't ask to allow user to specify env variable.
3. And this PR doesn't fix #263. If user doesn't specify TENV_WORKDIR than Ansible tasks will not know workdir.

Edited 5 years ago by astepano

ssahani commented 5 years ago

We do not want allow user to define workdir. Not now. Without real-test case that asks for it.

Why Not. martin given one

2. #263

ask to export of workdir with help of env variable to Ansible tasks. #263 - it doesn't ask to allow user to specify env variable.

Are you sure . How you do that

And this PR doesn't fix #263. If user doesn't specify TENV_WORKDIR than Ansible tasks will not know workdir.

@ martinpitt. Can you clarify what exactly is the use case

Edited 5 years ago by ssahani

ssahani commented 5 years ago

I think you need to give proper clarification for everything. Just saying we don't does not help anything at all. Why and what are the drawbacks if you allow

astepano commented 5 years ago

@bgoncalv @mvadkert please review.

bgoncalv commented 5 years ago

I agree, this change is not related to #263 .

#263 is for the tests to know where STR is storing the tests, and not to change where the tests are stored.

martinpitt commented 5 years ago

Correct, it's not related to issue #263. To the contrary -- right now, tests can still rely on /var/str/. With this, it might possibly be something else, so it's all the more important that tests can ask where the work dir is, instead of hardcoding /var/str/.

rebased onto 8198a21

5 years ago

ssahani commented 5 years ago

Thanks @martinpitt I have now modified the description.

astepano commented 5 years ago

@ssahani should I close this PR?

This PR doesn't solve #263
This PR is unwonted. What is a reason for this change?

ssahani commented 5 years ago

@ssahani should I close this PR?
No . This is a RFE now. Avoids hard coding.

This PR doesn't solve #263
This PR is unwonted. What is a reason for this change?

See my comment above.

astepano commented 5 years ago

@ssahani should I close this PR?
No . This is a RFE now. Avoids hard coding.
This PR doesn't solve #263
This PR is unwonted. What is a reason for this change?
See my comment above.

I read all above comments. I didn't find any reason for this change. Who did requested this change? What this change tries to solve? What is use case?

Please update documentation also that explains how to use TENV_WORKDIR variable with.

ssahani commented 5 years ago

I read all above comments. I didn't find any reason for this change. Who did requested this change? What this change tries to solve? What is use case?

We should not hard code any location. There should be a flexibility where to work dir.

Please update documentation also that explains how to use TENV_WORKDIR variable with.

Sure will do

astepano commented 5 years ago

We should not hard code any location. There should be a flexibility where to work dir.

@ssahani , I want to understand the purpose of this PR. Please notice, that above comments do not explain purpose of this PR.

Could you please answer on above questions:

What is use case for this PR?
Who did requested this change?
What tests has requested this change?
Has any STR user asked for this change?
What tests will use this variable?
There are other predefined, hard-coded variables. Why did you choose exactly this variable?

ssahani commented 5 years ago

What is use case for this PR?
Again see my comments above. . Choosing a work dir.

Who did requested this change?
What tests has requested this change?
Has any STR user asked for this change?
What tests will use this variable?
Any one can give a PR to enhance the functionality and work in a better manner.

There are other predefined, hard-coded variables. Why did you choose exactly this variable?
These are always can be reworked and enhanced and functionalist.

bgoncalv commented 5 years ago

This PR does not seem to cause any harm, but I don't understand why this is necessary? Has some one requested this feature? Why would someone need to change this directory in the VM?

I'd avoid adding this unless there is a valid case.

ssahani commented 5 years ago

This PR does not seem to cause any harm, but I don't understand why this is necessary? Has some one requested this feature?

Well it's not like someone going to request then we going to do. For example enhancement is a continues thing. Again for example I would like to my work dir in '/run/str' since this is temporary. After reboot it will be gone. I don't have to go manually delete them.

Why would someone need to change this directory in the VM?

We not every time running in VM . these can be run in laptops/servers anywhere.

I'd avoid adding this unless there is a valid case.

astepano commented 5 years ago

I'd avoid adding this unless there is a valid case.
+1 A real STR user who will use this new feature.

Edited 5 years ago by astepano

ssahani commented 5 years ago

What do you mean by you will decide ? Again What the mean real STR user . you mean you will decide who is fake who is real ?

dperpeet commented 5 years ago

The comments on this pull request are starting to veer away from being respectful and based on facts. Can we get back on track?

I'm happy to facilitate a discussion on the topic. From what I'm reading, @ssahani opened this pull request with a specific use case in mind.
@astepano @bgoncalv Based on the code, does this pull request match the intent, is that aligned with the STR guidelines / direction or do you have concrete change suggestions or reasons why this shouldn't be done at all?

Thank you!

bgoncalv commented 5 years ago

The PR seems to make sense for "I would like to my work dir in '/run/str'" scenario and I'm fine with it getting merged, we might want to document this somewhere.

astepano commented 5 years ago

@bgoncalv completely agree.

bookwar commented 5 years ago

@ssahani

I think that adding some flexibility to STR parameters (at least to define all paths used in a playbook) could be a valid enhancement). But this pull request doesn't actually achieve that. It provides only a hotfix for one particular case, not a generic solution.

I suggest that we create an issue where we can design the way we handle that.

Things to consider:

1) we should identify all global parameters which we want to have exposed as variables and maybe move them to a common empty role. It wouldn't do anything else then providing the common defaults for other roles.

This role can be then inherited by other roles of STR.

This way we can get a one dedicated place to manage all global variables.

2) Then we can consider ways of managing those parameters - getting them from environment, from config file, whatever..

3) if we go with the env variables. then we should define the namespace for them, for example use prefix like STR_, so that these variables doesn't overlap with generic env variables people might have in their environment.

And once you have a prefix, you can even do the magic and map all STR_<smth> variables to the "smth.lower()" ansible variables without hardcoding every variable individually.