fix(#119): add new home for configuration files #144

dipak-pawar · 2018-05-07T04:20:39Z

Fixes: #119

alien-ike · 2018-05-07T04:48:33Z

Ike Plugins (test-keeper)

Thank you @dipak-pawar for this contribution!

It appears that no tests have been added or updated in this PR.

Automated tests give us confidence in shipping reliable software. Please add some as part of this change.

If you are an admin or the reviewer of this PR and you are sure that no test is needed then you can use the command /ok-without-tests as a comment to make the status green.

Your plugin configuration is stored in the file.

centos-ci · 2018-05-14T16:10:28Z

Can one of the admins verify this patch?

MatousJobanek · 2018-05-15T08:38:50Z

Thanks @dipak-pawar for your work, unfortunately, I'm afraid that the task was misunderstood. The new default location of the config files .ike-prow/ should be something that is automatically checked for the presence of the files - for both the configuration file and plugin hint file (with that fact that the location of the plugin hint file can be still customized in config file). It should work in the same way as the .github directory works for the GH templates.
The pkg/assets/config/test-keeper.yaml should stay there where it was - this contains default inclusions and exclusions applicable for all projects the plugins are used in. The .ike-prow/test-keeper.yaml file contains additional configuration specific for the particular project - our own one is here: https://github.com/arquillian/ike-prow-plugins/blob/master/test-keeper.yaml that one should have been moved to the directory.

MatousJobanek

see my comment above

MatousJobanek · 2018-05-17T07:17:38Z

[test]

… into issue_119

dipak-pawar · 2018-05-17T07:47:33Z

@MatousJobanek can you look into this when you find some time?

MatousJobanek

Thanks for the change. So the config is already automatically loaded from the default location, it would be great if it would work for the plugin hint file in the same way as well.
Please take a look also at the test failures https://travis-ci.org/arquillian/ike-prow-plugins/builds/380075424?utm_source=github_status&utm_medium=notification

MatousJobanek · 2018-05-17T12:33:43Z

pkg/assets/config/test-keeper.yaml

@@ -134,3 +135,4 @@ skip_validation_for:

  # Our own configurations
  - 'regex{{test-keeper\.y[a]?ml$}}'


is this line still necessary when the configs should be inside of the directory that is excluded?

MatousJobanek · 2018-05-17T12:36:59Z

.ike-prow/test-keeper.yaml

@@ -0,0 +1,2 @@
+skip_validation_for: 
+  - 'cluster/'


there is still also the original file https://github.com/dipak-pawar/ike-prow-plugins/blob/c32f52a822cedb073f21298dda3e98ecb424bf82/test-keeper.yaml - it should be removed (moved here)

… into issue_119

MatousJobanek · 2018-05-22T12:49:24Z

pkg/github/service/config_loader.go

 // revision. Two files are expected to be found there plugin-name.yml or plugin-name.yaml (in that order)
 func (l *LoadableConfig) Sources() []config.Source {
 	return []config.Source{
-		l.loadFromRawFile("%s.yml"),
-		l.loadFromRawFile("%s.yaml"),
+		l.loadFromRawFile(".ike-prow/%s.yml"),


make .ike-prow/ a constant as it is used at several places

MatousJobanek · 2018-05-22T13:03:29Z

pkg/github/service/config_loader.go

+
+		if e == nil {
+			l.BaseConfig.PluginHint = githubBaseURL + rawFileService.GetRelativePath(pluginHintPath)
+		}


this shouldn't be here. This function is for loading configuration file, not the *_hint file.
In addition, on the line 51 you download the file - that should be lazy. Download it (and generate the default path) only when needed.
This should be part of comment_message.go

MatousJobanek · 2018-05-22T13:26:28Z

pkg/plugin/test-keeper/configuration_gh_test.go

@@ -24,8 +24,10 @@ var _ = Describe("Test keeper config loader features", func() {

 		It("should load test-keeper configuration yml file", func() {
 			// given
+			NonExistingRawGitHubFiles(".ike-prow/test-keeper_hint.md")


I don't think that this is necessary as it checks the suffix

… into issue_119

MatousJobanek · 2018-05-23T15:58:59Z

pkg/plugin/test-keeper/comment_message.go

@@ -40,6 +40,8 @@ func CreateCommentMessage(configuration PluginConfiguration, change scm.Reposito
 		msg = sadIke + paragraph + beginning + paragraph + noConfig
 	} else if configuration.PluginHint != "" {
 		msg = getMsgFromConfigHint(configuration, change)
+	} else if content := defaultFileContent(configuration, change); content != "" {
+		msg = content


there is one case that is not covered - when there is no config file, but the hint file is present.

MatousJobanek · 2018-05-23T15:59:45Z

pkg/plugin/test-keeper/comment_message.go

@@ -56,6 +58,18 @@ func getMsgFromConfigHint(configuration PluginConfiguration, change scm.Reposito
 	return configuration.PluginHint
 }

+func defaultFileContent(configuration PluginConfiguration, change scm.RepositoryChange) string {
+	pluginHintPath := fmt.Sprintf(ghservice.ConfigHome+"%s_hint.md", configuration.PluginName)


when formatting function is used, then use it for all parts:
fmt.Sprintf("%s%s_hint.md", ghservice.ConfigHome, configuration.PluginName)

MatousJobanek · 2018-05-23T16:01:12Z

pkg/plugin/test-keeper/event_handler_gh_test.go

@@ -80,10 +80,10 @@ var _ = Describe("Test Keeper Plugin features", func() {
 			gockEmptyComments(2)

 			gock.New("https://raw.githubusercontent.com").
-				Get(repositoryName + "/5d6e9b25da90edfc19f488e595e0645c081c1575/test-keeper.yml").
+				Get(repositoryName + "/5d6e9b25da90edfc19f488e595e0645c081c1575/" + ghservice.ConfigHome + "test-keeper.yml").


in configuration_gh_test.go you have extracted the config file path to a variable: https://github.com/arquillian/ike-prow-plugins/pull/144/files#diff-8881d5433bc67c80efc79906b968d297R25 use it same also here

MatousJobanek · 2018-05-23T16:03:22Z

Please also change the documentation to explain the "workflow" of loading the *_hint.md files - that when nothing is set in config, then it is automatically loaded from the pre-defined location.

… into issue_119

comment_message.go consistent

new home

MatousJobanek · 2018-05-24T11:48:31Z

@dipak-pawar please, take a look at my last changes - if it makes sense to you and you are fine with it.

dipak-pawar · 2018-05-24T11:52:18Z

@MatousJobanek Thanks for all changes. lgtm

MatousJobanek · 2018-05-24T12:02:33Z

[test]

alien-ike added the size/XS label May 7, 2018

dipak-pawar requested review from hemanik and MatousJobanek May 7, 2018 04:21

alien-ike added size/S and removed size/XS labels May 7, 2018

MatousJobanek requested changes May 15, 2018

View reviewed changes

chore: ignore .dockerignore and .ike-prow directory

cb5a4da

dipak-pawar added 2 commits May 17, 2018 12:48

chore: update path to take .ike-prow dir for configuration

7cb818a

chore: update doc for plugin hint configuration

a2c1115

dipak-pawar force-pushed the issue_119 branch from 067b46c to a2c1115 Compare May 17, 2018 07:29

dipak-pawar added 2 commits May 17, 2018 13:10

chore: move our test-keeper configuration in .ike-prow dir

96604b8

Merge branch 'master' of https://github.com/arquillian/ike-prow-plugins…

5e2f45c

… into issue_119

Merge branch 'master' into issue_119

c32f52a

MatousJobanek requested changes May 17, 2018

View reviewed changes

dipak-pawar added 4 commits May 18, 2018 11:23

chore: removes test-keeper.yaml from root dir

64e63f2

chore: removes unnecessary regex test-keeper.yaml from config

3acc1b8

fix: modify test to new use new config location

6ec0087

Merge branch 'master' of https://github.com/arquillian/ike-prow-plugins…

e4eb40c

… into issue_119

alien-ike added size/M and removed size/S labels May 18, 2018

chore: update test-fixture with new location for config file

1d3531f

chore: update glide.lock

5ccc521

alien-ike added size/L and removed size/M labels May 21, 2018

Merge branch 'master' into issue_119

3b3efa4

MatousJobanek requested changes May 22, 2018

View reviewed changes

dipak-pawar added 5 commits May 23, 2018 15:19

Merge branch 'master' of https://github.com/arquillian/ike-prow-plugins…

1821514

… into issue_119

fix: add const for .ike-prow/ and removes config file eager loading

1f3d490

fix: remove unnecasary code from test

bf096fd

fix: add comment for exported constant

fd03a90

chore: remove unnecessary mocking in test and use const variable

61fff53

MatousJobanek requested changes May 23, 2018

View reviewed changes

dipak-pawar and others added 12 commits May 24, 2018 10:57

chore: use format function for all part

12a8d5e

chore: extract configFilePath variable in test

f6e7fea

fix: add missing scenario - no config file, but hint file

cca4a27

doc: add doc for default loading behaviour of config hint file

5e5bd42

Merge branch 'master' of https://github.com/arquillian/ike-prow-plugins…

d584a4b

… into issue_119

fix: fix failing tests after merge

1530326

fix: modified CreateCommentMessage to not use nested else

9fd8d2e

modifies documentation

317e2f8

brings back the string in defaultFileContent method to keep the

264f938

comment_message.go consistent

removes location url in test case where it should be missing

6b6d360

changes documentation so the content in plugins chapters reflects the

856bd0a

new home

Merge branch 'master' into issue_119

78a84b4

removed unnecessary conversion

4611126

MatousJobanek approved these changes May 24, 2018

View reviewed changes

MatousJobanek merged commit 1748860 into arquillian:master May 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#119): add new home for configuration files #144

fix(#119): add new home for configuration files #144

dipak-pawar commented May 7, 2018

alien-ike commented May 7, 2018

centos-ci commented May 14, 2018

MatousJobanek commented May 15, 2018

MatousJobanek left a comment

MatousJobanek commented May 17, 2018

dipak-pawar commented May 17, 2018

MatousJobanek left a comment

MatousJobanek May 17, 2018

MatousJobanek May 17, 2018

MatousJobanek May 22, 2018

MatousJobanek May 22, 2018

MatousJobanek May 22, 2018

MatousJobanek May 23, 2018

MatousJobanek May 23, 2018

MatousJobanek May 23, 2018

MatousJobanek commented May 23, 2018 •

edited by dipak-pawar

Loading

MatousJobanek commented May 24, 2018

dipak-pawar commented May 24, 2018

MatousJobanek commented May 24, 2018

		@@ -134,3 +135,4 @@ skip_validation_for:

		# Our own configurations
		- 'regex{{test-keeper\.y[a]?ml$}}'

fix(#119): add new home for configuration files #144

fix(#119): add new home for configuration files #144

Conversation

dipak-pawar commented May 7, 2018

alien-ike commented May 7, 2018

Ike Plugins (test-keeper)

centos-ci commented May 14, 2018

MatousJobanek commented May 15, 2018

MatousJobanek left a comment

Choose a reason for hiding this comment

MatousJobanek commented May 17, 2018

dipak-pawar commented May 17, 2018

MatousJobanek left a 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

MatousJobanek commented May 23, 2018 • edited by dipak-pawar Loading

MatousJobanek commented May 24, 2018

dipak-pawar commented May 24, 2018

MatousJobanek commented May 24, 2018

MatousJobanek commented May 23, 2018 •

edited by dipak-pawar

Loading