platform-complete stripes building issue fix addition to previous tic…

[eldiiar-duishenaliev]

…ket: FOLIO-3768

[zburke]

What this script does and what it says it does in the header comments do not agree. The header comments say it builds a bundle, but what it actually does is (1) update yarn.lock based on changes in package.json and then (2) build a bundle. If we accept the implementation as the true purpose, then this PR isn't right, even though it contains changes I suggested (running yarn install --frozen-lockfile on release branches).

• On a release branch, which should update yarn.lock with changes from package.json while leaving other dependencies alone, it should leave an existing yarn.lock in place and install deps with yarn install.
• On a development branch, which should update yarn.lock with the newest versions of all dependencies, it should remove yarn.lock and install deps with yarn install.

The only time to run yarn install --frozen-lockfile is if we are trying to exactly replicate a previous build by using the same package.json and yarn.lock files without making any changes. For example, I would expect the scripts that construct the nightly reference envs or the bugfest envs to operate this way.

[zburke]

Removing the trailing $ enables branches like R2-2023-Consortia, but is a lot more broad than that, too. Do we want to be that broad?

[zburke]

We should be using branches named like 2023-R2... instead of R2... that do not sort correctly. Is this an opportunity to correct that gaffe or has that ship already sailed?

[zburke]

See also folio-org/platform-complete/pull/2600. master must be treated as a release branch because it references released versions like 5.4.0 in package.json, not ranges like ^5.4.0 that can be satisfied with CI versions. This means we need to update the "don't remove yarn.lock" conditional to include master as well.

[eldiiar-duishenaliev]

See also folio-org/platform-complete/pull/2600. master must be treated as a release branch because it references released versions like 5.4.0 in package.json, not ranges like ^5.4.0 that can be satisfied with CI versions. This means we need to update the "don't remove yarn.lock" conditional to include master as well.

master branch included in condition.

[zburke]

@eldiiar-duishenaliev, I am not a Groovy guy. Are you confident about this conditional?

if (!(branch =~ /^[rR]\d-\d{4}(-([rR][cC]|hotfix-\d))?$/ || /\bmaster\b/)) {

I would expect it to evaluate it as !(branch operator regex || regex) and thus it would always fail since a non-null regex evaluates as truthy. I think we need to include the master-branch condition within a single regex, or have two conditions here.

[eldiiar-duishenaliev]

@eldiiar-duishenaliev, I am not a Groovy guy. Are you confident about this conditional?

if (!(branch =~ /^[rR]\d-\d{4}(-([rR][cC]|hotfix-\d))?$/ || /\bmaster\b/)) {

I would expect it to evaluate it as !(branch operator regex || regex) and thus it would always fail since a non-null regex evaluates as truthy. I think we need to include the master-branch condition within a single regex, or have two conditions here.

Sir, I've tested it before making a commit:

[zburke]

@eldiiar-duishenaliev, Sorry, I don't think I explained that well. The condition is structured as if (! (foo)) so we need foo to evaluate to false to match the if condition but that never happens. foo resolves to regex || /\bmaster\b/, so it doesn't matter how the regex resolves because /\bmaster\b/ is a constant which always evaluates to true. IOW, the inner condition is always true and since it gets negated, only the else block here is ever executed.

Your test showed the logic working correctly with master; good. Try it with r2-2023, good. But try it with snapshot or feature-branch or monkey: those should execute the if block but they don't.

[eldiiar-duishenaliev]

@eldiiar-duishenaliev, Sorry, I don't think I explained that well. The condition is structured as if (! (foo)) so we need foo to evaluate to false to match the if condition but that never happens. foo resolves to regex || /\bmaster\b/, so it doesn't matter how the regex resolves because /\bmaster\b/ is a constant which always evaluates to true. IOW, the inner condition is always true and since it gets negated, only the else block here is ever executed.

Your test showed the logic working correctly with master; good. Try it with r2-2023, good. But try it with snapshot or feature-branch or monkey: those should execute the if block but they don't.

Thanks @zburke !
Really appreciate your support!
Smal adjustment was pushed to this PR, looks much better now :)

[zburke]

Awesome, f70a361 LGTM.

Are we confident about changing yarn install to yarn install --frozen-lockfile? It means that when this script runs on a release branch, it will only be capable of replicating a previous build, and there must be some other script that updates yarn.lock when we want to bump @folio/whatever from 4.2.6 to 4.2.7. Is there such a script? If yes, great. If now, we must leave the yarn install command alone for now.

[eldiiar-duishenaliev]

Hello @zburke
Would you please share your feedback?
Are we OK to merge this PR?
Thanks in advance.