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

Rendering maven.env fails #18

Closed sroegner closed 7 years ago

sroegner commented 7 years ago

Executing the formula on existing pillar data now results in this error:

Rendering SLS 'base:maven.env' failed: Jinja variable 'salt.pillar object' has no attribute 'user'

In a recent addition to the env.sls file there is a direct call to the pillar (instead of the already imported settings.sls which normally deals with resolving pillars and grains). PLEASE - i do not see myself introducing a global /user pillar variable - i have more than one user. This should be added to settings.sls and pulled out of the maven object in env.sls.

noelmcloughlin commented 7 years ago

Ahh yes, this is technical debt shared by a number of formulae I touched recently, and pillar lookups should be consolidated into settings. Scale-up scenarios bring pattern of few diverse states wanting top share some contextID's like some user. I'm not sure if static configuration (Pillar) or runtime configuration lookup/Jinja2 is solution yet. Thanks for raising.

noelmcloughlin commented 7 years ago

Regarding shared access to a 'user' variable (whatever its called) here are options. The scenario of states sharing a user or users means pillar is needed.

  1. Splitting maven.env state-
  1. Alternative is assume pillar has variable 'default_user' or primary_user & check if defined using Jinja. This removes the bug.

  2. Any other ideas.

@sroegner thanks.

sroegner commented 7 years ago

My main issue is with the lack of scoping - not the name. Also: i have a hard time considering this technical debt when it causes my provisioning to fail as a result.

noelmcloughlin commented 7 years ago

@sroegner This bug was assigned to me, please cancel your PR, since it was not agreed to change ownership and I've just prepared a PR for this which is consistent across intellij / sqldeveloper / eclipse / maven. I've done the work.

sroegner commented 7 years ago

Just for the record - we needed the formula fixed right away and so i submitted a PR - then saw that @noelmcloughlin had done the same. I closed mine and merge his. Sorry I didn't mean to offend - this was just a lack of communication.

noelmcloughlin commented 7 years ago

Stefen. In hindsight I acted wrongly here in overreacting to minor thing - I apologize.

Please capture your comments re. bad implementation in RFE issue so we can improve this - and I'm happy to do the work - but I'm new to Jinja2.

The key question is - how do we tell salt/jinja2 to iterate over POSIX users? This I would welcome help and suggestions towards @sroegner @whiteinge @blbradley This will be for Maven, Intellij, Sqlplus, sqldeveloper, eclipse-java, etc.

The Pillars are the only Salt Information Model I know about so thats why I used to store a "default_user: undefined_user" variable but I agree with Stefen.

I have reopened this issue as reminder followup is needed!!

Thanks.

whiteinge commented 7 years ago

Thanks for working things out, guys.

It's hard to catch regressions without automated testing. Hopefully bringing kitchen-salt into Salt core will help make setting that up easier and more common in the future. Without that double-checking for things like syntax errors is all the more important. The settings.sls file is a solid pattern to use but not universally used across formulas so it's easy to overlook -- I'm sure I missed it when merging a PR a few weeks ago, so sorry about that.

Again, thanks for setting things right and calming things down. There are benefits and drawbacks to having an informal dance around contributing to these repos and sometimes toes will get stepped on. The demeanor of the Salt community continues to impress me.

sroegner commented 7 years ago

@noelmcloughlin To your question on user settings: I don't believe it buys us much to just have a way of checking for users to exist or listing them - the current code also makes a hard-coded assumption about the home-directory of a user (being /home/name) when for example the jenkins home more typically resides under /var/lib/jenkins and other users might exist but never had a home or lost it.

I believe a starting point could be to simply use a pillar list of usernames and then try to find a way to properly deal with their non-existence or that of their home-directories. Doing this right is hard because it means catching a lot of cases without pushing too much logic into jinja - I say that as soemone who is definitely guilty of doing that. One way of doing that could be to unite all the developer-minded formulas into one that has sub-modules for sqldeveloper, eclipse and so forth and which could share could via macros - I just used that kind of style in here: https://github.com/sroegner/java-toolbox-formula. It is not meant as a prescription but an example of what this could look like structurally.

Hth

noelmcloughlin commented 7 years ago

@blbradley I deferred another test of #20 until the morning but PR was closed almost immediately, and I assumed community encouraged a PR lifecycle. I had to push to master instead.

@sroegner New features will bring technical debt but implementation can evolve over time. I have stakeholder giving broad requirements and I either need to capture all scenarios first time to satisfy reviewers OR make assumptions, and I choose the latter as best for productivity reasons. My firm UC is a developer community who want to personalize their OS installation so current implementation supports but activates toggle.

Here is bug for /home https://github.com/saltstack-formulas/maven-formula/issues/22

I have hundreds of developers to cater for and I'm struggling to understand what needs to be done to evolve further. Thats why I pinged @sroegner @blbradley @whiteinge because the UC needs to be defined - otherwise a solution is not possible.

Thanks.

blbradley commented 7 years ago

@noelmcloughlin

I have hundreds of developers to cater for and I'm struggling to understand what needs to be done to evolve further.

I'm not sure if you're referring to your own customers or the SaltStack community. Which is it?

noelmcloughlin commented 7 years ago

@sroegner

i do not see myself introducing a global /user pillar variable - i have more than one user.

I'm not sure if you're referring to your own customers or the SaltStack community. Which is it?

sroegner commented 7 years ago

@noelmcloughlin I was talking about our in-house pillar data, thus clearly not referring to anything Community - to me it appears to make sense that the formulas have to function in a multi-user server context and that single-user development environments are a very specific case of that.

noelmcloughlin commented 7 years ago

Are you going to raise enhancement issue for the functionality you see missing? You need to advocate for your UC's too.

I have resolved your original problem and its transparent to formula users now. I agree things can evolve for multi-user development environments but apart from cycling over POSIX users on the target minion its totally unclear to me. For UCs demanded from me, solution fits for now.

br.

sroegner commented 7 years ago

Are you going to raise enhancement issue for the functionality you see missing? You need to advocate for your UC's too.

@noelmcloughlin I am not sure what you're expecting here: new functionality that you needed resulted in code changes which ended up breaking the maven.env state. You appear to be the stakeholder and if this works for you and everyone else then maybe it is time to close this thread and move on.

sroegner commented 7 years ago

Verified manually using Salt 2016.11.6 (Carbon) on CentOS 6.9 Thanks for the fixes @noelmcloughlin

noelmcloughlin commented 7 years ago

@sroegner I would like to know the UC you have forcefully and passionately argued for? Are you telling everyone now you have no UC?

sroegner commented 7 years ago

@noelmcloughlin I am unaware of being forceful or overly passionate and apologize for appearing that way to you. My additional remarks above were meant as feedback towards a potential solution beyond "technical debt" but I only meant to suggest what one possible solution could look like. I have nothing in terms of what you keep calling a "UC" and I think that this github issue has served it's purpose - I would appreciate it very much if you could refrain from re-opening it again.