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

Added global configuration #69

Merged
merged 6 commits into from
Aug 31, 2015
Merged

Added global configuration #69

merged 6 commits into from
Aug 31, 2015

Conversation

jimbethancourt
Copy link
Contributor

Added capability to have a global configuration that will provide a default configuration for all projects that do not have a repository-level configuration. Repository configuration will override global configuration if present.

…nagement screen. Global configuration is overridden by repository YACC hook configuration. Tested successfully against Stash 3.10.2
Moved building of settings from the YaccServletConfig into YaccPreReceiveHook
Fixed infinite loop in YaccConfigServlet.doPost()
Updated unit test mocks in YaccPreReceiveHookTest to reflect updated YaccPreReceiveHook code
@jimbethancourt jimbethancourt changed the title Fixed config screen Added global configuration and fixed config screen Aug 18, 2015
@jimbethancourt jimbethancourt changed the title Added global configuration and fixed config screen Added global configuration Aug 18, 2015
String parameterName = (String) key;

// Plugin settings persister only supports map of strings
if (!parameterName.startsWith("errorMessage") && !parameterName.equals("submit")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was doing some testing and noticed that the customized error messages aren't being saved. I tracked it down to this !parameterName.startsWith("errorMessage"), which appears to skip the customized error messages. Do you have some background on why this check is here? If I remove it, the header/footer settings get saved correctly. The other custom error messages are saved (and show up in commit reject messages), but don't get re-populated in the config form for some reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had noticed that the error request parameters weren't saved in PR#38 and had followed that logic -- I hadn't given any thought to it. I will definitely take !parameterName.startsWith("errorMessage") out. I'll try to recreate the issue regarding the re-population of errors and see if I can figure it out.

Thanks!
Jim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good news and bad news -- taking !parameterName.startsWith("errorMessage") out, the header/footer settings and the other custom error messages were correctly saved and repopulated (in Stash 3.10.2).

The biggest concern I have is that the Issue JQL Matcher error message doesn't pop up an error message or redirect to a very useful error page -- it only shows a "500 Error" skull and crossbones page, at least for the local instance I have that doesn't have a JIRA application link set up. It looks like this is a limitation of Stash itself looking at https://answers.atlassian.com/questions/229401/error-page-decorator-or-template-in-stash The error message shows up in the Stash logs, but the 500 page doesn't indicate this... How concerned are you about this?

I will proceed with committing and pushing the Config servlet with !parameterName.startsWith("errorMessage") removed.

Thanks,
Jim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to wrap the call to validate() in a try/catch and see if I can catch the exception and put it at the top of the serverside template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woohoo! I'm catching the InvalidStateException and reporting it as a field error, which now shows up on the config form both for the global and repository configurations, giving an immediate and clear error message. I'll include this in the push as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only common thread I can find about the fields that aren't being repopulated is that they are all of type YaccError and they have a period in their field name.
However, I realize that changing anything relating to configuration would wipe out the current configuration for current YACC users...

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what version of Stash it starts working in? If it is something low, one option is to up the required version to that (depending on usage stats).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure -- I can take a bisection approach and start with Stash 3.5 Unfortunately atlas-run doesn't seem to want to co-operate for me, but thankfully downloading and setting up Stash is quick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The earliest version it works on is 3.5.x -- I tested on 3.2.7, 3.3.5, 3.4.5, and 3.5.1 How do the usage numbers look on your end?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking out the versions! Approximately 20-25% of YACC users are using <3.5.1. At the same time approximately 1/3 of users are still using YACC 1.4 and below. My gut tells me that the same people who are using <3.5.1 are probably also using an older YACC version. So, while 20-25% is a fair amount of users, I am not sure how many of them are actually keeping YACC updated. So, I think bumping the required version to 3.5.1 wouldn't be a problem, especially if we are gaining global configuration.

…exist and reporting it as a field error on the configuration form.

Now populating custom error messages.
…e it clears any valdation messages that may have occurred on previous save / cancel action
sford added a commit that referenced this pull request Aug 31, 2015
@sford sford merged commit d93470b into mohamicorp:master Aug 31, 2015
@sford sford mentioned this pull request Aug 31, 2015
@sford
Copy link
Contributor

sford commented Aug 31, 2015

I merged this, thanks for all your effort! I upped the minimum supported Stash version to 3.5.1. #64 reminded me that supporting Stash 4.0 will require upping the Stash version to at least 3.7, so it isn't a bad thing to get the minimum supported version up :-)

@jimbethancourt
Copy link
Contributor Author

Thanks Sean!
It has been a fantastic experience working with you. :-) Thanks for not
minding bumping up the version number. Hopefully it will make things easier
for a lot of folks.

Thanks,
Jim

On Sun, Aug 30, 2015, 8:11 PM Sean Ford notifications@github.com wrote:

I merged this, thanks for all your effort! I upped the minimum supported
Stash version to 3.5.1. #64
#64 reminded me
that supporting Stash 4.0 will require upping the Stash version to at least
3.7, so it isn't a bad thing to get the minimum supported version up :-)


Reply to this email directly or view it on GitHub
#69 (comment)
.

@aramalipoor
Copy link

👍

@sford sford added this to the NEXT milestone Sep 13, 2015
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.

3 participants