saltstack-formulas / maven-formula

A minimalistic way to install just the maven distribution without any dependencies
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
4 stars 10 forks source link

syntax error in settings.sls #21

Closed sroegner closed 7 years ago

sroegner commented 7 years ago
`local:
Data failed to compile:
----------
Rendering SLS 'base:maven' failed: Jinja syntax error: unexpected '}', expected ')'
/var/cache/salt/minion/files/base/maven/settings.sls(40):
---
[...]
{%- set source_hash = default_source_hash %}
{%- else %}
{%- set source_hash = g.get('source_hash', p.get('source_hash', default_source_hash )) %}
{%- endif %}

{%- set user          = g.get('default_user', salt['pillar.get']('default_user', p.get('default_user', 
default_user))%}    <======================
{%- set orgdomain     = g.get('orgdomain', p.get('orgdomain', default_orgdomain )) %}
{%- set scmhost       = g.get('scmhost', p.get('scmhost', default_scmhost )) %}
{%- set repohost      = g.get('repohost', p.get('repohost', default_repohost )) %}
{%- set archetypes    = g.get('archetypes', p.get('archetypes', default_archetypes )) %}`
sroegner commented 7 years ago

@noelmcloughlin please consider executing/testing your modified code before submitting a PR

noelmcloughlin commented 7 years ago

@sroegner You raised issue, assigned to @noelmcloughlin & then done your own thing without nametagging me - so I got no alerts whilst doing work meet both concerns ... #18 was assigned to me but I get my first surprise with #19 appearing from you!!

Today another surprise!!!!

Gone: https://github.com/saltstack-formulas/maven-formula/issues/18 Gone: https://github.com/saltstack-formulas/maven-formula/pull/19 Gone: https://github.com/saltstack-formulas/maven-formula/pull/20

If there is a bug is settings.sls its because the PR was not discussed. When I do bigger PR I do regression testing on Fedora/Suse/Ubuntu and this was not done as #20 indicates????

Please stop this behaviour now!! @sroegner

What you should have done is said "calm down, oops sorry for not nametagging you, lets commit #19 and you can rebase because #20 should isolate 'user' stuff" which I agree now is best approach .. => maven.userenv .. the outcome is we fix both our concerns (bug & technical debt).

This did not happen and we are where we are!

noelmcloughlin commented 7 years ago

Fix verified on Ubuntu 17.04, OpenSUSE Leap, Fedora 26 @sroegner

blbradley commented 7 years ago

@noelmcloughlin

When I do bigger PR I do regression testing on Fedora/Suse/Ubuntu and this was not done as #20 indicates????

@sroegner mentioned that you should execute your change before submitting a PR, and I'm inclined to agree with him. I have not seen @sroegner show any bad behavior, so he does not need to 'stop'. Try not to take comments on GitHub so personally.

noelmcloughlin commented 7 years ago

@blbradley its not collaboration in my opinion - did you not read my summary of @sroegner behaviour above - and give your opinion on him? @whiteinge

sroegner commented 7 years ago

@noelmcloughlin I am sorry that there seems to be no effective way of clearing up what has to be a misunderstanding. I had to spend a lot of time lately investigating regressions introduced into a formula that happens to be part of our stack but has been unusuable to us for quite some time now. I understand that your experience might be different - i.e it working for you.

As to what you perceive as my behavior I am - again - sorry things went this way but I was following what used to be the workflow here. Having said that I do find it a reasonable expectation to have at least executed a formula/state before submitting a PR: others finding your syntax errors is simply not what the pull request process is for.

@blbradley To not let this escalate any further I will leave the project and maintain my own fork from now on. Sorry again for the trouble

blbradley commented 7 years ago

@sroegner No trouble. I don't use this formula. Feel free to stay.