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 setup/ directory #1620

Merged
merged 5 commits into from
Oct 17, 2019
Merged

Remove setup/ directory #1620

merged 5 commits into from
Oct 17, 2019

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Dec 1, 2018

Alternative to #1277, just delete everything that's not relevant. Although I can't speak for the AIX config, @mhdawson are we OK to ditch that one or is it still used?

www is moved to ansible/www-standalone and I've updated the README.md in there. That leaves us with no setup/ directory.

@richardlau
Copy link
Member

Although I can't speak for the AIX config, @mhdawson are we OK to ditch that one or is it still used?

Michael is on holiday. Ping @gdams ?

@rvagg rvagg force-pushed the rvagg/remove-old-setup branch from 459f1b6 to 5ab1d5e Compare April 20, 2019 00:59
@rvagg
Copy link
Member Author

rvagg commented Apr 20, 2019

rebased, this should blow away setup/ on the current version, @nodejs/build ping

@rvagg rvagg force-pushed the rvagg/remove-old-setup branch from 5ab1d5e to f6eefd9 Compare April 20, 2019 01:28
@mhdawson
Copy link
Member

The AIX setup is still used so I'd want us to keep that.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

We need to keep the AIX directory contents for now.

@rvagg rvagg force-pushed the rvagg/remove-old-setup branch from f6eefd9 to 79e92b3 Compare April 24, 2019 00:51
@rvagg
Copy link
Member Author

rvagg commented Apr 24, 2019

backed out the aix61 deletion, but @mhdawson maybe we should move it to ansible as aix61-standalone, that's what I've done with www

@mhdawson
Copy link
Member

@sam-github can you look at @rvagg suggestion?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@sam-github
Copy link
Contributor

Can someone explain a bit of the history here? I assume the aix61 manual setup is manual just because it predates ansible, or perhaps no one had time to add the configuration steps to ansible?

For www, its likely not a class of machine covered by the current playbooks, so I can see why its a specific playbook.

I'm not clear why there would be a "standalone" AIX playbook. We don't do that for any other jenkins clients. Shouldn't the contents of the setup/aix61/ansible-playbook.yaml be in the regular ansible playbook for jenkins clients, with exclusion tags, as it is done for other platforms?

@mhdawson If you would like me to rewrite this, I'll need a scratch AIX6.1 machine to provision. I doubt I should be running possibly broken playbooks under development on an existing AIX machine, and even if I didn't break the machine, I wouldn't really know the playbook works if the setup has already been applied to that machine manually.

@rvagg
Copy link
Member Author

rvagg commented Apr 24, 2019

Sorry @sam-github, it is a bit mired in history. We started out here with everything standalone in the setup/ directory before Johan did a rewrite-the-world refactor and put everything under ansible/. The common (most recent) platforms went straight in to that when it launched but older ones were either deferred or left undone entirely (like centos5). There's been talk of integrating www but at this stage I think we'd rather just reengineer www entirely than have to patch it into our new ansible fancyness.

There's no strict need to remove setup/, it was more of a housekeeping thing and I have no problems keeping setup/aix61 but it'll be in its own limbo which might give a wrong impression. VersionSelectorScript says that 61 is still in use across the all active versions so I suppose this is current hardware. It's really your call if you want to leave it standalone or try and integrate it into the new ansible setup. As a standalone we could just move it into ansible as aix61-standalone so it can be friends with www-standalone and be a project for some future date, if at all.

@rvagg
Copy link
Member Author

rvagg commented Apr 24, 2019

to clarify, moving it to ansible/aix61-standalone is literally just a move, nothing else and the hope is that "-standalone" is a clear indicator that this thing isn't integrated.

@sam-github
Copy link
Contributor

OK, thanks, I think I understand how things stand.

@sam-github
Copy link
Contributor

sam-github commented Oct 16, 2019

I looked at #1620 (comment), and I don't think its worth doing. Its not just a move, the playbook depends on a var file that has to go somewhere, and on two files in the setup/aix61/resources/ directory. The playbook also won't run against current machine names (I tried with --check after moving it), it needs its hosts specifier updated, and then the search paths for its deps changed, and then it would need testing... but we don't have any new AIX6.1 machines to test against and aren't likely to get them.

All of which is to say, they serve as documentation for how the AIX6.1 machines were originally setup, but at this point aren't runnable and I don't think they should moved. AIX6.1 is EOL, and eventually (not soon, but its coming) will be removed from CI, at which point the directory can be removed. For now, lets just leave it alone.

@rvagg rvagg force-pushed the rvagg/remove-old-setup branch from 79e92b3 to abb255b Compare October 16, 2019 22:15
@rvagg
Copy link
Member Author

rvagg commented Oct 16, 2019

@sam-github the point of this was to remove the setup/ directory entirely to avoid the confusion of what it's about. I was proposing moving the entire aix61 directory into ansible/aix61-standalone, so all of the files are preserved, resources and all, it's just a move so we can remove the directory. See latest commit for such a move. If it's still in use that's fine, we can just remove it later.

I also see we have a setup/aix71.md, I'm going to PR a separate proposal to put that material into doc/non-ansible-configuration-notes.md where I think it might be best suited. Once that's done we can ditch the directory and save the confusion it continues to cause.

@sam-github
Copy link
Contributor

I was proposing moving the entire aix61 directory into ansible/aix61-standalone

I misunderstood. What you describe above is totally reasonable, go for it.

@rvagg rvagg merged commit fbcbd6c into master Oct 17, 2019
@rvagg rvagg deleted the rvagg/remove-old-setup branch October 17, 2019 23:29
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.

5 participants