Fixes #26411 - Inline bastion as an engine #8032

jlsherrill · 2019-03-19T15:29:51Z

No description provided.

theforeman-bot · 2019-03-19T15:29:58Z

Issues: #26411

jlsherrill · 2019-03-19T15:32:26Z

First commit pulls in bastion as is in katello/bastion master, second commit adapts it to an engine.

jlsherrill · 2019-03-26T00:49:25Z

[test katello]

engines/bastion/lib/bastion/engine.rb

jlsherrill · 2019-03-27T01:04:10Z

@waldenraines @johnpmitsch one thing that @ehelms brought when chatting about this PR, is if we want to do a gitsubtree to try to preserve all the history, or just merge it as it is (all in one commit). Curious your thoughts

jlsherrill · 2019-03-27T01:11:37Z

Here's a todo list:

fix/update rake bastion:ci and rake bastion:setup
after merge, update infrastructure to use new rake bastion:ci/setup

My current thinking is to modify it so that:

rake bastion:setup
rake bastion:ci

sets up and runs tests for 'bastion', while:

rake bastion:katello:setup
rake bastion:katello:ci

does this for bastion_katello. Thoughts?

johnpmitsch · 2019-03-27T12:05:41Z

@waldenraines @johnpmitsch one thing that @ehelms brought when chatting about this PR, is if we want to do a gitsubtree to try to preserve all the history, or just merge it as it is (all in one commit). Curious your thoughts

I don't have much experience with git sub-modules or sub-trees so its hard for me to say. My initial thoughts are if it introduces more complexity we should just merge it as is. I imagine the bastion repo will still be around (but defunct) and that will still have the original commit history? But maybe I don't see the benefits, would the submodule be beneficial for packaging reasons?

rake bastion:katello:setup
rake bastion:katello:ci
does this for bastion_katello. Thoughts?

Having these as rake tasks always felt odd to me. Can we consider adding the grunt tasks to package.json as npm scripts? We could additionally add one npm command to do all javascript (both react and angular) testing too. I think it would be another step towards modernizing and unifying this part of the code with the react code. Just a thought, I'm not opposed to the rake task syntax either.

jlsherrill · 2019-03-27T13:30:21Z

I don't have much experience with git sub-modules or sub-trees so its hard for me to say. My initial thoughts are if it introduces more complexity we should just merge it as is. I imagine the bastion repo will still be around (but defunct) and that will still have the original commit history? But maybe I don't see the benefits, would the submodule be beneficial for packaging reasons?

It would just be for preserving history. Imagine importing all of bastion and its ~1500 commits directly into katello within a subdirectory. The only thing we gain is commit history, but we'd be adding ~1500 commits to the tree. The files on the file system of master would end up looking exactly the same.

Having these as rake tasks always felt odd to me. Can we consider adding the grunt tasks to package.json as npm scripts? We could additionally add one npm command to do all javascript (both react and angular) testing too. I think it would be another step towards modernizing and unifying this part of the code with the react code. Just a thought, I'm not opposed to the rake task syntax either.

That sounds reasonable, can you provide any pointers on how to do that? I'm happy to look into it.

johnpmitsch · 2019-03-27T13:51:17Z

That sounds reasonable, can you provide any pointers on how to do that? I'm happy to look into it.

sure thing, you would want to replicate the commands in Bastion's Rakefile in the script section in Katello's package.json

This is what I'm thinking:

{
  "bastionTests: "cd ./bastion && grunt ci",
  "bastionSetup": "cd ./bastion && ./setup_npm",
  "allJavascriptTests: "npm run bastionSetup && npm run bastionTests && npm test" // maybe have another one with no setup
}

Then add a setup npm script as executable ./setup_npm in bastion folder that performs these steps, you could keep it as ruby or just do a bash script

johnpmitsch · 2019-03-27T13:55:54Z

It would just be for preserving history. Imagine importing all of bastion and its ~1500 commits directly into katello within a subdirectory. The only thing we gain is commit history, but we'd be adding ~1500 commits to the tree. The files on the file system of master would end up looking exactly the same.

I get the general concept, I'm just failing to see the benefit. The commits would be preserved on the original Bastion repo assuming we are keeping it around, so if people want the commits for professional reasons, they are still there. It seems like more complexity for no clear reason but I'm open to being persuaded otherwise! :)

jlsherrill · 2019-03-27T15:05:18Z

@johnpmitsch thats the way i was leaning as well, just wanted to open it up for discussion. Thanks!

waldenraines · 2019-03-27T18:21:46Z

Here's a todo list:

fix/update rake bastion:ci and rake bastion:setup

after merge, update infrastructure to use new rake bastion:ci/setup

My current thinking is to modify it so that:
rake bastion:setup
rake bastion:ci
sets up and runs tests for 'bastion', while:
rake bastion:katello:setup
rake bastion:katello:ci
does this for bastion_katello. Thoughts?

Having these as rake tasks always felt odd to me. Can we consider adding the grunt tasks to package.json as npm scripts? We could additionally add one npm command to do all javascript (both react and angular) testing too. I think it would be another step towards modernizing and unifying this part of the code with the react code. Just a thought, I'm not opposed to the rake task syntax either.

I agree with this and we could take it a step further to replace bower entirely with a package.json and npm scripts if we ended up moving bastion into webpack.

For now I think we should do whatever is easiest to maintain functionality although I would also argue that the actual rake tasks themselves could go away.

@waldenraines @johnpmitsch one thing that @ehelms brought when chatting about this PR, is if we want to do a gitsubtree to try to preserve all the history, or just merge it as it is (all in one commit). Curious your thoughts

I personally don't care about the history. Half of the history is "Bump bastion to version x" anyway. If we ever need the history we can check out the old repo and look at it there or on github.

ehelms · 2019-03-28T00:53:38Z

The difference with preserving commits is the use of tools like git log or tig or blame to trace the history within the git repository itself. The original repository can be explored, it will just be more work for developers. Just adding some color.

ehelms · 2019-03-28T01:35:43Z

I played around and I think we can keep things as is, simplify and fix the CI setup with:

diff --git a/engines/bastion_katello/package.json b/engines/bastion_katello/package.json
index e732431..ba649fe 100644
--- a/engines/bastion_katello/package.json
+++ b/engines/bastion_katello/package.json
@@ -1,4 +1,7 @@
 {
   "name": "bastion_katello",
-  "version": "1.0.0"
+  "version": "1.0.0",
+  "dependencies": {
+    "bastion": "file:../bastion"
+  }
 }

And this PR for infra -- theforeman/foreman-infra#1006
The infra PR can be merged ahead of time as the changes will be needed not just for testing this PR but to ensure the PR test job is backwards compatible.

I'd also ask for the following preparation PRs to have:

PR to bastion that deprecates the repository via a README update
PR to foreman-packaging that removes Bastion plugin and updates Katello's spec file

jlsherrill · 2019-03-28T01:40:52Z

@ehelms i like this as it makes things simple and you've done part of the work :)

let me work on those things, thanks!

ehelms · 2019-03-28T01:49:26Z

I forgot to mention you can then just drop those Rakefiles.

…

On Wed, Mar 27, 2019, 9:40 PM Justin Sherrill ***@***.***> wrote: @ehelms <https://github.com/ehelms> i like this as it makes things simple and you've done part of the work :) let me work on those things, thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8032 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAR584K0IBbWyRQccED3726ozSTT4OUKks5vbB2kgaJpZM4b8byh> .

jlsherrill · 2019-03-28T01:54:56Z

theforeman/foreman-packaging#3559
https://github.com/Katello/bastion/pull/245/files

waldenraines · 2019-03-28T13:10:03Z

I forgot to mention you can then just drop those Rakefiles.

I guess we'd also want to update the developer documentation if we're no longer going to support rake bastion:setup. Also, we should pull this documentation into katello's setup documentation too I guess.

ehelms · 2019-03-28T16:00:51Z

[test katello]

engines/bastion/lib/bastion/engine.rb

johnpmitsch · 2019-03-28T18:08:55Z

engines/bastion/README.md

@@ -188,13 +188,13 @@ Open the file, and add the following lines (with empty lines above and below for
 To setup a development environment, clone the repository or your fork of the repository. From the git checkout, setup the required development dependencies by running:

 ```
-rake bastion:setup
+npm install


Does only npm install work to setup everything?

looking at theforeman/foreman-packaging#3559

it looks like it does, except for this step:
sh "cp -rf \$(cat foreman/bastion-version) engines/bastion_katello/bastion-${bastion_version}"
@ehelms would it make sense to just create a symlink there so we don't have to do that step?

I only ask because it looks like more steps, but not sure if they are redundant. I can test it out soon too

Oh and one more step here: "npm install bastion-${bastion_version}". Which i'll add to the docs

@ehelms could this be replaced with just 'npm install ../bastion' ?

i pushed some updated docs, and added simple instructions to ./engines/katello_bastin/README.mc

Does only npm install work to setup everything?

I suppose that I forgot we commit all vendor files to source control so there is no need to run bower unless you're updating a dependency.

It should be possible to remove bower entirely since we aren't really updating dependencies that often and replace it with npm and webpack in a later PR.

johnpmitsch · 2019-03-28T18:11:47Z

engines/bastion/README.md

 ```

 After making changes, tests and linting can be run via:

 ```
-rake bastion:ci
+grunt ci


Do you plan on adding these to Katello's package.json?

after @ehelm's suggestion, i decided to not do that (sorry should have mentioned that here). I'd rather get this in quickly and then we can adjust if desired. Does that sound okay?

no worries, gotta draw the line somewhere :) I guess there is a difference between testing bastion and bastion_katello and I'm forgetting this too!

I'll try to do a follow up PR adding these to package.json, maybe we could have a single npm run ci command to run everything we expect jenkins to do as well.

Just be cautious given they both have their own package.json with dependencies and pathing configured based on their relative locations. Mixing these with the base katello dependencies and scripts might be more more mashing together than you necessarily want. If anything, I'd think you'd be better off investing in dropping all of the Grunt infrastructure in favor of native scripts (but even that may not be worth the time if the webpack/React is the future).

ehelms

I can't see any other issues and CI is happy.

waldenraines

LGTM

johnpmitsch

Great change!

tbrisker · 2019-03-29T21:01:16Z

We're now getting a cyclic failure when trying to build the katello rpm, could it be related to this change?
http://koji.katello.org/kojifiles/work/tasks/6423/186423/build.log

jlsherrill · 2019-03-29T21:33:31Z

attempting to handle this here: theforeman/foreman-packaging#3559

theforeman-bot added Needs testing Not yet reviewed labels Mar 19, 2019

ehelms reviewed Mar 26, 2019

View reviewed changes

engines/bastion/lib/bastion/engine.rb Outdated Show resolved Hide resolved

jlsherrill force-pushed the bastion_rm_1 branch from 6079ae5 to ebd4d3f Compare March 27, 2019 02:04

jlsherrill marked this pull request as ready for review March 27, 2019 02:04

jlsherrill mentioned this pull request Mar 28, 2019

Remove bastion requirement from katello theforeman/foreman-packaging#3559

Merged

3 tasks

jlsherrill force-pushed the bastion_rm_1 branch from ebd4d3f to 5495499 Compare March 28, 2019 02:56

ehelms reviewed Mar 28, 2019

View reviewed changes

engines/bastion/lib/bastion/engine.rb Outdated Show resolved Hide resolved

jlsherrill force-pushed the bastion_rm_1 branch from 5495499 to 63806ae Compare March 28, 2019 17:15

jlsherrill changed the title ~~[WIP] Fixes #26411 - Inline bastion as an engine~~ Fixes #26411 - Inline bastion as an engine Mar 28, 2019

Fixes #26411 - Migrating bastion gem into katello

4c35548

jlsherrill force-pushed the bastion_rm_1 branch from 63806ae to bb33cb6 Compare March 28, 2019 17:51

johnpmitsch reviewed Mar 28, 2019

View reviewed changes

Fixes #26411 - Adapt bastion to an engine

638d020

jlsherrill force-pushed the bastion_rm_1 branch from bb33cb6 to 638d020 Compare March 28, 2019 19:02

ehelms approved these changes Mar 28, 2019

View reviewed changes

waldenraines approved these changes Mar 28, 2019

View reviewed changes

johnpmitsch approved these changes Mar 29, 2019

View reviewed changes

jlsherrill merged commit 5c1a0d0 into Katello:master Mar 29, 2019

jlsherrill deleted the bastion_rm_1 branch March 29, 2019 13:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #26411 - Inline bastion as an engine #8032

Fixes #26411 - Inline bastion as an engine #8032

jlsherrill commented Mar 19, 2019

theforeman-bot commented Mar 19, 2019

jlsherrill commented Mar 19, 2019

jlsherrill commented Mar 26, 2019

jlsherrill commented Mar 27, 2019

jlsherrill commented Mar 27, 2019 •

edited

johnpmitsch commented Mar 27, 2019

jlsherrill commented Mar 27, 2019

johnpmitsch commented Mar 27, 2019

johnpmitsch commented Mar 27, 2019

jlsherrill commented Mar 27, 2019

waldenraines commented Mar 27, 2019

ehelms commented Mar 28, 2019

ehelms commented Mar 28, 2019

jlsherrill commented Mar 28, 2019

ehelms commented Mar 28, 2019 via email

jlsherrill commented Mar 28, 2019

waldenraines commented Mar 28, 2019

ehelms commented Mar 28, 2019

johnpmitsch Mar 28, 2019

jlsherrill Mar 28, 2019 •

edited

johnpmitsch Mar 28, 2019

jlsherrill Mar 28, 2019

jlsherrill Mar 28, 2019

jlsherrill Mar 28, 2019

waldenraines Mar 28, 2019

johnpmitsch Mar 28, 2019

jlsherrill Mar 28, 2019

johnpmitsch Mar 28, 2019

ehelms Mar 28, 2019

ehelms left a comment

waldenraines left a comment

johnpmitsch left a comment

tbrisker commented Mar 29, 2019

jlsherrill commented Mar 29, 2019

Fixes #26411 - Inline bastion as an engine #8032

Fixes #26411 - Inline bastion as an engine #8032

Conversation

jlsherrill commented Mar 19, 2019

theforeman-bot commented Mar 19, 2019

jlsherrill commented Mar 19, 2019

jlsherrill commented Mar 26, 2019

jlsherrill commented Mar 27, 2019

jlsherrill commented Mar 27, 2019 • edited

johnpmitsch commented Mar 27, 2019

jlsherrill commented Mar 27, 2019

johnpmitsch commented Mar 27, 2019

johnpmitsch commented Mar 27, 2019

jlsherrill commented Mar 27, 2019

waldenraines commented Mar 27, 2019

ehelms commented Mar 28, 2019

ehelms commented Mar 28, 2019

jlsherrill commented Mar 28, 2019

ehelms commented Mar 28, 2019 via email

jlsherrill commented Mar 28, 2019

waldenraines commented Mar 28, 2019

ehelms commented Mar 28, 2019

Choose a reason for hiding this comment

jlsherrill Mar 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehelms left a comment

Choose a reason for hiding this comment

waldenraines left a comment

Choose a reason for hiding this comment

johnpmitsch left a comment

Choose a reason for hiding this comment

tbrisker commented Mar 29, 2019

jlsherrill commented Mar 29, 2019

jlsherrill commented Mar 27, 2019 •

edited

jlsherrill Mar 28, 2019 •

edited