Issues/250 - Implement shell hook plugin #319
Conversation
|
The TravisCI tests are failing with: I have no idea what's causing this. |
| (PLAN_COMMAND): new HookPoint(PLAN_COMMAND), | ||
| (APPLY): new HookPoint(APPLY), | ||
| (APPLY_COMMAND): new HookPoint(APPLY_COMMAND) | ||
| ] |
There was a problem hiding this comment.
Would it be easier if this were initialized as an empty map? You're iterating over all the hooks to check if the shells have been set for the hooks. If you had the map empty, and only initialized the map when a hook point were added for the given key (ALL/INIT/PLAN, etc...) - you'd only be iterating over the hooks that actually have values.
I think it might make both your testing and implementation simpler. <-- that's a maybe. It might not.
There was a problem hiding this comment.
On the flip side, it means that withHook() would need to null-check each of the values, and construct the new HookPoint objects if they're needed. Maybe this is backwards, but I wanted to contain all of the logic of "is this hook set" inside the HookPoint class, which led to the current implementation. Though I'm certainly open to changing it if you prefer.
There was a problem hiding this comment.
To be more specific, I think there's a lot of examples like this, where you're iterating over all the hooks because they're all non-null. Looping can be tricky in a test - there's actually no guarantee that a PLAN_COMMAND entry will even exist in the list.
@Test
void setRunBefore() {
TerraformEnvironmentStageShellHookPlugin.withHook(PLAN_COMMAND, 'foo bar', WhenToRun.BEFORE)
def hooks = TerraformEnvironmentStageShellHookPlugin.hooks
hookKeys.each {
if (it == PLAN_COMMAND) {
assertTrue(hooks[it].isConfigured())
assertEquals(hooks[it].runBefore, 'foo bar')
assertNull(hooks[it].runAfterAlways)
assertNull(hooks[it].runAfterOnFailure)
assertNull(hooks[it].runAfterOnSuccess)
} else {
assertFalse(hooks[it].isConfigured())
}
}
}
I was wondering if it might be more straight forward to do something like this, which obviates the need to loop, and by passes the problem of the element not being in the list.
@Test
void setRunBefore() {
TerraformEnvironmentStageShellHookPlugin.withHook(PLAN_COMMAND, 'foo bar', WhenToRun.BEFORE)
def hooks = TerraformEnvironmentStageShellHookPlugin.hooks
assertEquals(1, hooks.size())
def planHooks = hooks[PLAN_COMMAND]
assertTrue(planHooks.isConfigured())
assertEquals(planHooks.runBefore, 'foo bar')
assertNull(planHooks.runAfterAlways)
assertNull(planHooks.runAfterOnFailure)
assertNull(planHooks.runAfterOnSuccess)
}
Which then suggests even more simplification with:
@Test
void setRunBefore() {
TerraformEnvironmentStageShellHookPlugin.withHook(PLAN_COMMAND, 'foo bar', WhenToRun.BEFORE)
def planHooks = TerraformEnvironmentStageShellHookPlugin.getHooks(PLAN_COMMAND)
assertTrue(planHooks.isConfigured())
assertEquals(planHooks.runBefore, 'foo bar')
assertNull(planHooks.runAfterAlways)
assertNull(planHooks.runAfterOnFailure)
assertNull(planHooks.runAfterOnSuccess)
}
It's a suggestion though, just based on looking at the tests, and not looking at the implementation.
There was a problem hiding this comment.
So... those alternatives are missing the intended point of the test, or half of it.
@Test
void setRunBefore() {
TerraformEnvironmentStageShellHookPlugin.withHook(PLAN_COMMAND, 'foo bar', WhenToRun.BEFORE)
def hooks = TerraformEnvironmentStageShellHookPlugin.hooks
hookKeys.each {
if (it == PLAN_COMMAND) {
assertTrue(hooks[it].isConfigured())
assertEquals(hooks[it].runBefore, 'foo bar')
assertNull(hooks[it].runAfterAlways)
assertNull(hooks[it].runAfterOnFailure)
assertNull(hooks[it].runAfterOnSuccess)
} else {
assertFalse(hooks[it].isConfigured())
}
}
}
Is intended to test that the plan before hook, and ONLY the plan before hook, has been set. The iteration is there to ensure that none of the other hooks, even though they're present, have been touched. If there was an easy way to serialize the complete state of hooks and test against that, then I'd do it.
The iteration here was just intended to simplify:
assertFalse(TerraformEnvironmentStageShellHookPlugin.getHooks(INIT_COMMAND).isConfigured())
assertFalse(TerraformEnvironmentStageShellHookPlugin.getHooks(PLAN).isConfigured())
def planHooks = TerraformEnvironmentStageShellHookPlugin.getHooks(PLAN_COMMAND)
assertTrue(planHooks.isConfigured())
assertEquals(planHooks.runBefore, 'foo bar')
assertNull(planHooks.runAfterAlways)
assertNull(planHooks.runAfterOnFailure)
assertNull(planHooks.runAfterOnSuccess)
assertFalse(TerraformEnvironmentStageShellHookPlugin.getHooks(APPLY).isConfigured())
assertFalse(TerraformEnvironmentStageShellHookPlugin.getHooks(APPLY_COMMAND).isConfigured())
On another note:
there's actually no guarantee that a PLAN_COMMAND entry will even exist in the list.
Why not? Am I missing something?
There was a problem hiding this comment.
The iteration is there to ensure that none of the other hooks, even though they're present, have been touched
That's where I was suggesting that using nulls might be helpful to simply that intent.
def hooks = TerraformEnvironmentStageShellHookPlugin.hooks
assertEquals(1, hooks.size())
...
def planHooks = hooks[PLAN_COMMAND]
...
The existence of a single non-null hook confirms that all other hooks are not set. No worries though, it was just a thought.
Is intended to test that the plan before hook, and ONLY the plan before hook, has been set.
On another note:there's actually no guarantee that a PLAN_COMMAND entry will even exist in the list.
Why not? Am I missing something?
Apparently, I missed something, and not you. I didn't realize until now that you created a hookKeys array, and are iterating over THAT, and not iterating over the hooks - so I'm wrong.
class TerraformEnvironmentStageShellHookPluginTest {
def hookKeys = [ALL, INIT_COMMAND, PLAN, PLAN_COMMAND, APPLY, APPLY_COMMAND]
...
def hooks = TerraformEnvironmentStageShellHookPlugin.hooks
--> hookKeys.each {
if (it == PLAN_COMMAND) {
assertTrue(hooks[it].isConfigured())
assertEquals(hooks[it].runBefore, 'foo bar')
assertNull(hooks[it].runAfterAlways)
assertNull(hooks[it].runAfterOnFailure)
assertNull(hooks[it].runAfterOnSuccess)
} else {
assertFalse(hooks[it].isConfigured())
}
}
There was a problem hiding this comment.
The existence of a single non-null hook confirms that all other hooks are not set. No worries though, it was just a thought.
Ah, ok, got it. So part of my concern was that since we're just using string keys here (albeit static final string variables instead of using the strings directly), to ensure that only the correct hook was configured, even if others existed as non-null. Granted, I suppose that's only a concern with the specific current implementation.
Apparently, I missed something, and not you. I didn't realize until now that you created a hookKeys array, and are iterating over THAT, and not iterating over the hooks - so I'm wrong.
Ah, ok, yeah.
I don't think that's related to issue #313. I can't reproduce locally, but I did find the same error referenced elsewhere. |
|
Looks like JobDSL 1.77 upgraded groovy to 2.4.12. The CHANGELOG for 2.4.12 notes a fix for compile errors in legacy version.
Upgrading groovy seems to make the problem go away. Upgrading gradle reveals the problem locally (which doesn't make sense, since travisCI is using ./gradlew - same as what we'd be using locally). At any rate, I'll cut a difficult issue to address this. See Issue #321 |
|
Also, can you update the CHANGELOG for this PR? |
|
@jantman Merged |
|
@kmanning Rebased and tests passing. Thanks!! |
Codecov Report
@@ Coverage Diff @@
## master #319 +/- ##
==========================================
+ Coverage 79.06% 79.13% +0.07%
==========================================
Files 41 43 +2
Lines 1065 1107 +42
Branches 247 262 +15
==========================================
+ Hits 842 876 +34
Misses 99 99
- Partials 124 132 +8
Continue to review full report at Codecov.
|
3 participants
This PR is an initial, proof-of-concept implementation of a shell script hook plugin for TerraformEnvironmentStage, as described in #250. The plugin name (
TerraformEnvironmentStageShellHookPlugin) is intended to be an indication that this is not an attempt at an all-encompassing solution to the hook problem, and is one initial implementation that may or may not inspire something at a higher level.The code included here has full unit test coverage, and has also been tested in a Jenkinsfile on a private Jenkins instance. All seems to be working well.