remove fetch-includes early return

[ekohilas]

I previously came across an issue where I was getting incorrect output due to cached files.

On realising this, I set the --fetch-includes flags, as it was documented that it would always fetch includes even when cached.

However when this flag was enabled, the program returned no output, and short-circuited all other flags.

This is because of the changes seen below, even though this flag value is used elsewhere to determine whether includes should be refetched.

While this is an api breaking change, if I understand the code correctly, it as an acceptable bug fix.

This is because the flag currently doesn't produce any output and only calls Parser.create.

However this PR would set it so that the flag does produce the correct output (which if I understand correctly won't make a difference as the user isn't checking for output) and all other code paths call Parser.create anyway.

If this change is not acceptable, the creation of a new flag would be required, along with changes to the rest of the code that uses fetchIncludes so that it performs as expected.

[ANGkeith]

i don't really see it as an bug, seemed like --fetch-includes was intentionally written to be a two step process

gcl --fetch-includes          # to force update the cache
gcl                           # re-run gcl using the updated cache

but i do like the behavior you're proposing as it sounds cleaner and more intuitive...

just like docker build vs docker build --no-cache

[ekohilas]

i don't really see it as an bug, seemed like --fetch-includes was intentionally written to be a two step process

gcl --fetch-includes          # to force update the cache
gcl                           # re-run gcl using the updated cache

Oh I see. If that's the case, that's fine, it should be better documented then so users can be aware that that is the case.

but i do like the behavior you're proposing as it sounds cleaner and more intuitive...

just like docker build vs docker build --no-cache

Yes, the other advantage is that if there was a certain configuration that was required (such as setting certain variables and options) then they need to be specified twice, in addition to having to re-process everything again.

[firecow]

It definently was on purpose. "normally" programmers specify hardpinned include dependencies.
Only developers actively working on a module/include would use have to use --fetch-includes, because they utilize "master/latest/branches"

[ekohilas]

It definently was on purpose. "normally" programmers specify hardpinned include dependencies. Only developers actively working on a module/include would use have to use --fetch-includes, because they utilize "master/latest/branches"

Ah, thanks I see.

So what's the outcome of this?

1. Should the PR be closed, and documentation updated such that scripts that want to always fetch includes instead run:

gitlab-ci-local --fetch-includes --variable VAR=test
gitlab-ci-local --variable VAR=test --list-json

2. Should the PR be changed to add a new argument that bypasses the changes in the current PR but still sets the flag elsewhere?
3. Rename the existing flag to be more specific?
4. Something else?

[ANGkeith]

@firecow,
what do you think about this mr ?

seemed to be quite a popular behavior,

requested by #1371 too

[firecow]

I'm ok with it.. 👍