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

Add this.callers_promisers variable #2301

Conversation

amousset
Copy link
Contributor

Ref: https://dev.cfengine.com/issues/7493

With methods, we can re-utilize common configuration pieces from different places. But it is currently impossible to identify, within the called bundle, what is the
context of the call, and hence difficult to produce meaningful logs or reporting, particularly when bundles are called with the same arguments.

A way to make it possible to know the context would be to create a new context variable, for example called this.callers_promisers, which would be a list of the parents promisers of the current bundle.

This way we can use the full call stack to identify uniquely the current action.

@cfengine-review-bot
Copy link

Can one of the admins verify this patch?

@tzz
Copy link
Contributor

tzz commented Aug 20, 2015

This is a brilliant idea, but I would rather make it @this.context@ and a data container so it can hold all the information about the stack. Do you think that's possible?

@jooooooon
Copy link
Contributor

Intresting idea, @tzz. What other information about the stack are you thinking of? I see at least the names of calling bundles as well (this currently is just promisers, not bundle names), but can't think of anything else right now.

@tzz
Copy link
Contributor

tzz commented Aug 20, 2015

I would include everything I can from the StackFrame structure:

  • iteration state at each level (VERY useful for debugging)
  • call path of bodies and bundles and promises

Plus

  • promise source file and line

Because it's a data container, the format is flexible so it can get even more useful over time.

@nickanderson
Copy link
Member

I like the idea of it being a data container with more info. And I think this is a great feature.

@tzz
Copy link
Contributor

tzz commented Aug 23, 2015

Also I would add the parameters passed to each body or bundle or promise along the stack path.

@amousset please let us know if you're interested in working on this.

@amousset
Copy link
Contributor Author

amousset commented Sep 1, 2015

I tried using a data container, but found out it was difficult to extract the data I needed in CFEngine code (namely the ordered list of parent methods promisers). I can still add more variablesto this PR (like bundle names) to cover more use-cases.

@tzz
Copy link
Contributor

tzz commented Sep 9, 2015

@amousset can you start with a data container (JSON data structure) to hold the call stack? That's a simple change. Make it a callers list of key-value maps with just one key right now:

[ { promiser: "a123" }, { promiser: "b456" }, ...]

I think it's a good change; see json.h for the API. But if you think that's too complicated, I can take your code and adapt it.

After that change, others can keep augmenting this variable since it's a JSON data container and keys can be inserted at will.

@peckpeck
Copy link
Contributor

With this approach it is complicated to extract the promiser stack.

With the slist approach I can do:

"trace" string => join(" / ", "this.callers_promisers")

I'm not against the use of data container, but to expand their usage, you need convenient functions to extract data, by example a function like this:

"stack" slist => map_data_to_list("this.callers_promisers", "${this}[promiser]")

@tzz
Copy link
Contributor

tzz commented Sep 14, 2015

@peckpeck I agree 100%, hence you should vote for https://dev.cfengine.com/issues/7435 by watching it. It's the best way to get what you're describing. In 3.7, the mapdata() function can already extract a specific key from a list of data containers, but jq support will turbocharge that, and it should be available in 3.8 together with the feature discussed here.

@amousset ping?

@peckpeck
Copy link
Contributor

I will, jq is a very good feature!
But I'm still missing a function that easily maps a data container to a list.

@tzz
Copy link
Contributor

tzz commented Sep 14, 2015

(Sorry for the digression, but it's related to this PR)

@peckpeck yes, mapdata will do it. Here's an example that will pick out the user key of the users data container.

      "users" data => '
         [
            { "user": "joe",
              "hash": "*KRB*", 
              "uid" : "1357",
              "promisee": "sysadmin team",
            }, 
            { "user": "bob",
              "hash": "*LCK*",
              "uid" : "1354",
              "promisee": "sysadmin team",
            } 
         ]
     ';

      "userkeys" data => mapdata("none", '$(users[$(this.k)][user])', users);
      "userkeys_slist" slist => getvalues(userkeys);

@kacf
Copy link
Contributor

kacf commented Sep 24, 2015

I think there is nothing wrong with doing both. Having a plain list is certainly convenient, even if a data container is also available. How about $(this.callers) for the data container variant, and $(this.callers_promisers) for a list of plain promiser names. After all, "promiser" does in fact mean the promise string, and nothing else.

@kacf
Copy link
Contributor

kacf commented Sep 24, 2015

(Then we could also split the work and not do both at once)

@kacf kacf self-assigned this Sep 24, 2015
@tzz
Copy link
Contributor

tzz commented Sep 25, 2015

Please consider #2331 which augments this PR with much more detail as I proposed.

@peckpeck and others, your help with #2319 would be very welcome. It's taking a while because I don't know Autoconf well. Once it's available, jq support will make manipulating complex data much easier.

@kacf
Copy link
Contributor

kacf commented Sep 28, 2015

Ok, closing this one in favor of #2331 then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants