CLDSRV-937: Drop --ignore-optional from Dockerfile#6203
Conversation
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
DarkIsDude
left a comment
There was a problem hiding this comment.
I'm not sure this is the right approach. Optional one "must" be optional. They should not trigger an exeception in the code. That's not really the philosophy of npm about them 😬. We have now several solutions:
- Remove them from optional in arsenal (and package them all the time, that can make sense as we expose them). For example some deployment don't use mongo, but we package it.
- Keep them optional and find how to fix them in arsenal. But I'm not sure we can as we need to trigger the tracing configuration at the root of the start.
- Declare them as a dependency in Cloudserver (so only installed by the components that need them) and keep your "optional" philosophy. Not sure it's really useful, as on other component that use arsenal, they don't have the ignore optional and so we'll install them even if we don't use the opentelemetry.
In any case, dropping the ignore here is a good idea (as you describe in your PR, it's the only one). But the fix for opentelemetry should not be this one.
eef56af to
20bb7d3
Compare
|
@DarkIsDude Fixing OTEL in scality/Arsenal#2653 , reworded this PR to be plain flag drop without the OTEL rationale |
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
This pull request does not target the following hotfix branch(es) so they
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
Queue build failedThe corresponding build for the queue failed:
Remove the pull request from the queue
|
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-937. Goodbye delthas. |
Summary
Drop the
--ignore-optionalflag fromDockerfile:30. There's no documented intent behind it, it's not used by any other Scality service, and removing it has no observable downside on the current dep tree.Archeology — why is the flag here?
Added 2019-08-01 in commit
81bce441b:That's the whole commit message. The flag was new behaviour in the npm→yarn migration — the prior
npm install --productiondid not skip optionals, so this wasn't a port of an existing constraint. The Dockerfile has been touched many times since (line moves, dep re-orderings, multi-stage build), but the flag itself has never been revisited or referenced anywhere.Other Scality services don't use this flag
Backbeat, for example:
No
--ignore-optional. Cloudserver was the outlier — there's no project-wide convention behind it.Doesn't break anything
Full list of
optionalDependenciesreachable from the current dep tree:ioctlsetDirSyncFlagon the Linux file metadata backend only (production Zenko uses bucketd / MongoDB, never the file backend). Already wrapped intry/catchwith a warning fallback. Builds cleanly — the builder stage already installsbuild-essential,python3,node-gyp,libffi-dev,zlib1g-dev.fsevents"os": ["darwin"]field. No-op.uglify-js,commander,source-map,encoding,@pkgjs/parseargs@opentelemetry/{sdk-node, sdk-trace-base, resources, exporter-trace-otlp-http}dependenciesin ARSN-601, so will be installed regardless of this flag once that lands.yarn.lockalready contains fullresolvedURLs andintegrityhashes for everything — with--frozen-lockfile, yarn just fetches the existing locked tarballs. Nopackage.jsonoryarn.lockchange in this PR.Related: ARSN-601
Issue: CLDSRV-937