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

remove the NODE_TEST_DIR env variable on Ubuntu 16.04 #658

Closed
wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Mar 22, 2017

Solves some recent problems with readable-stream and CITGM.

NODE_TEST_DIR is not present in the other environments.

For some context:

nodejs/node#11760 (comment)
nodejs/node#11760 (comment).

cc @MylesBorins @jasnell

@jbergstroem
Copy link
Member

We added this to solve cross-filesystem and disk size issues. Although it might not be needed for ubuntu16, I think consistency across the fleet is desired. We haven't rolled this out on all machines (since we don't redeploy on every change to playbooks), but it will likely happen sooner or later. We didn't bother creating the directory as part of a playbook since the nodejs test suite added it. Should we not just add the directory instead?

@mcollina
Copy link
Member Author

@jbergstroem I'm happy with adding the directory. I thought that given it was not on the full fleet, it was a mistake.

I'm happy with having the directory. The whole point is making sure that the directory exists if the env variable is there.

@jbergstroem
Copy link
Member

@mcollina perhaps citgm can create the tmp folder if it doesn't exist, similarly to the node test suite? I can add the folder as part of the playbook in parallel.

@mcollina
Copy link
Member Author

@jbergstroem it's not a problem of CITGM for that. It's in https://github.com/nodejs/readable-stream/blob/master/test/common.js#L45, which is lifted from core https://github.com/nodejs/node/blob/master/test/common.js#L35-L36. So, it is the node test suite.

I would rather prefer not to do a readable-stream release just to change test/common.js

@piccoloaiutante
Copy link
Member

@mcollina @jbergstroem how about adding here this:

- name: Creates NODE_TEST_DIR directory
  file: path=/home/{{ server_user }}/tmp state=directory

Instead of this pr change?

@jbergstroem
Copy link
Member

@piccoloaiutante i'd suggest adding in a more generic role, like a java worker. The tap2junit python script is more related to the conversion of output; not so much just running a java worker.

@piccoloaiutante
Copy link
Member

@jbergstroem shouldn't be better to add it as part of an already existing role likebootstrap? We just only need to create the missing folder for ubuntu machine, so we could add it in ubuntu partials here.

@jbergstroem
Copy link
Member

@piccoloaiutante exactly. It should live with the jenkins client role.

@piccoloaiutante
Copy link
Member

should we close this in favour of #785 @mcollina ?

@mcollina mcollina closed this Jul 23, 2017
@mcollina
Copy link
Member Author

Closed!

@gibfahn
Copy link
Member

gibfahn commented Jul 24, 2017

should we close this in favour of #785 @mcollina ?

In general I'd say leave it open, and add the Fixes: line in #785, that way the issue stays open even if the PR doesn't land for some reason. Doesn't really matter either way though.

@mcollina mcollina reopened this Jul 24, 2017
gibfahn pushed a commit that referenced this pull request Sep 11, 2017
This PR tries to adapt the previous #658 to the new ansible structure
that we adopted recently.

Basically it adds an extra step for Ubuntu 16.04 and 16.10 to create
the tmp directory used in NODE_TEST_DIR variable.
@gibfahn gibfahn closed this Sep 11, 2017
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.

4 participants