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

Cleanup #242

Merged
merged 42 commits into from
Jan 25, 2019
Merged

Cleanup #242

merged 42 commits into from
Jan 25, 2019

Conversation

pcarney8
Copy link
Contributor

No description provided.

@springdo springdo self-requested a review November 21, 2018 03:23
@springdo
Copy link
Contributor

This could be hard to validate in the labs-ci-for-ci-cd. I remember when I made big changes to the Jenkinsfile last time, i had to work around some stuff to validate changes to the ci/cd project. If you want when I am back from Hols, would be great to go through some of this stuff together?

@pcarney8
Copy link
Contributor Author

@springdo yeah, let me know when you have time. A lot of the whole "using regex" will no longer be needed and a lot of things will be getting chopped out of here. This branch is very much a work in progress at the moment, but I'm modeling off of an existing repo that I've used for a customer and that repo is currently in working order, so I believe getting something working will be fairly straightforward. Additional pieces that need to be solved first, new repos for some of the things that got chopped

@pcarney8 pcarney8 requested review from oybed and sherl0cks January 2, 2019 22:21
@pcarney8
Copy link
Contributor Author

pcarney8 commented Jan 2, 2019

@springdo it's time to get this merged in. there are a lot of changes here, but re-creating and re-running the Jenkinsfile is very easy. If you want to have a call to talk about it, let me know.

Copy link
Contributor

@oybed oybed left a comment

Choose a reason for hiding this comment

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

Just a quick inline comment for now ... Also curious to know the reasoning for the name site.yml for the playbook(?) (sorry if it is already explained and I just didn't see it in my quick scan of the changes)

@pcarney8 pcarney8 changed the title WIP - Cleanup Cleanup Jan 3, 2019
@pcarney8
Copy link
Contributor Author

pcarney8 commented Jan 3, 2019

also, @oybed i left some secrets stuff in there (not currently working, i corrupted the encryption on purpose) because i would really like to have an example of using a secret in this. But I'm thinking that since we have the infrastructure repo with the labs-robot secret stuff that I'll probably just do it there or open up a specific private repo here on GH. thoughts?

@pcarney8
Copy link
Contributor Author

pcarney8 commented Jan 4, 2019

nvm, deleting the secrets from here.

@rdebeasi
Copy link
Contributor

Hi folks! This PR is two months old as of yesterday. In the spirit of continuous incremental improvement, I'd like to propose that this PR be merged as-is.

There are a couple of projects that could use the features in here, and right now the only way to get those features is to work off of @pcarney8's fork instead of an official Labs CI/CD release. Having to choose between fragmentation and missing features is a bummer.

I know there's probably some stuff that could use further refinement, but those changes could be done as incremental improvements in the future. Heck, you could even add some improvements before you cut the next release if you wanted.

Thanks to everyone for their work building and reviewing this PR! I know you all are juggling several things at once and it can be hard to make time for projects like this. 🚀 😄 🍕

@pcarney8
Copy link
Contributor Author

@oybed bump. you have time to review again, or no?

@rdebeasi I'm with you. i would like to merge this

@sherl0cks
Copy link
Contributor

@pcarney8 lets just move forward. make a tag or something before you merge so its easy to revert

@oybed
Copy link
Contributor

oybed commented Jan 25, 2019

@pcarney8 didn't know you were waiting on me for this one - I never really reviewed it in the first place, just some notes from a quick scan. Anyway - this is a major change, and I think it's probably a waste of time to try to test this before it's merged, so if we have consensus on just merging and fix any issues, I'm fine with that. Sounds like we are, so I'll cut a new release and merge.

Copy link
Contributor

@oybed oybed left a comment

Choose a reason for hiding this comment

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

Per discussion, due to size of change, agreed to merge as-is and fix issues post-merge.

@oybed oybed merged commit fa50ee6 into rht-labs:master Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants