This is achieved by reading a JSON object from a special environment variable, and then propagating that object all the way to all codeql invocations

This has lead to a lot of churn passing variables around. Since all that's saved is reading an environment variable and running JSON.parse, is it worth it reading it once per action and passing it around? I think it would be cleaner to just reading it when needed when running the codeql command.

setting the options through additional inputs in each codeql-action action instead of a single shell variable

I'm not sure this would be a nicer experience unfortunately. The inputs to actions are only allowed to be stirng values, so you'd still have to have encoded json in a string, in yaml. If the feature is intended to be slightly under the radar then not putting it in the action inputs seems good to me, so keeping it as an environment variable is fine.

the feature is intended for internal codeql users

I think this is unfortunately likely to be wishful thinking and not possible to enforce in reality. For users who read the code and discover the feature I agree we can be slightly justified in saying we hold no responsibility for it's behaviour and compatibilty. I'm more worried about when setting this option inevitably solves a customer issue, and then we have to keep supporting it because it's used in a customer's production build.

Thanks, but what does this feedback mean for this PR?

• It sounds like I just should move the environment variable read to its actual use sites?
• What should the environmen variable be named? Is the current CODEQLACTION_EXTRA_OPTIONS good enough, or do we want the more precise CODEQLACTION_CODEQL_EXTRA_OPTIONS?

Replies

I think it would be cleaner to just reading it when needed when running the codeql command.

It certainly be a smaller change. But do we want to rely on the command line environment so far away from the command line entry point?

I'm not sure this would be a nicer experience unfortunately. The inputs to actions are only allowed to be stirng values...

Ah. Good point.

I'm more worried about when setting this option inevitably solves a customer issue, and then we have to keep supporting it because it's used in a customer's production build.

Right. Don't we already have a record of semi-official LGTM_ environment variables though?
If the customer do reach about such a problem, I think a PR that introduces a proper action-input solution to their problem is merited. Without this environment variable backdoor, a fix would not even be possible without a new action release.

It sounds like I just should move the environment variable read to its actual use sites?

It certainly be a smaller change. But do we want to rely on the command line environment so far away from the command line entry point?

I'm not entirely sure and I don't feel all that strongly either way. I have a slight preference for reading it where needed and avoiding the extra method parameters, but I'm happy to go with whatever you want. My feelings on this are mostly because it's an environment parameter.

For this case in particular I don't think reading it deep in the code is a problem. However for things like the action inputs we will have to move those to all be read at the entrypoint. One of things we'll be working towards soon is making this run fully outside of actions and adding a CLI to it. So things that are actions-specific will need to be worked around. I don't think that affects this as it's an environment variable, but I can see we might want to take a similar approach and read it once and pass it around.

Don't we already have a record of semi-official LGTM_ environment variables though?
If the customer do reach about such a problem, I think a PR that introduces a proper action-input solution to their problem is merited. Without this environment variable backdoor, a fix would not even be possible without a new action release.

I don't think we have any semi-official LGTM_ environment variables. If you mean the LGTM_INDEX_INCLUDE and friends then we do use those but users aren't expected to know about them or set them themselves. We set them by interpreting the paths and paths-ignore config properties. So that would be an example of what you mean of making it official by giving it an action input. I think we can do that in the future though and don't have to do it right now.

Sorry I forgot to reply to this one

What should the environmen variable be named? Is the current CODEQLACTION_EXTRA_OPTIONS good enough, or do we want the more precise CODEQLACTION_CODEQL_EXTRA_OPTIONS?

There isn't really any precedent for this. I think call it whatever you want. I suppose CODEQLACTION_CODEQL_EXTRA_OPTIONS is the most descriptive, if you can accept having a name that long.

All comments addressed. Rebased to resolve compile conflict in codeql.ts

Rebased to resolve the compile conflict. Merging since green.