-
Notifications
You must be signed in to change notification settings - Fork 125
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
PMM-12641 Optimize PMM build scripts #3239
base: v3
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## v3 #3239 +/- ##
==========================================
- Coverage 43.45% 43.42% -0.04%
==========================================
Files 360 360
Lines 43977 43977
==========================================
- Hits 19112 19098 -14
- Misses 23204 23218 +14
Partials 1661 1661
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7a0ce14
to
19f1801
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggested more descriptive names
Co-authored-by: Catalina A <94133018+catalinaadam@users.noreply.github.com>
@@ -4,7 +4,6 @@ ARG VERSION | |||
ARG BUILD_DATE | |||
|
|||
ENV LANG=en_US.utf8 | |||
ENV LC_ALL=en_US.utf8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that we don't need it anymore? I remember there were issue with PostgreSQL. I hope it won't affect upgrade from PMM 2 to PMM 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pretty sure. From the logs, the OS emits a good number of warnings not being able to apply just LC_ALL
. However, once the language package is installed, we see that the default OS language settings are set, as expected, to en_US.utf8
. Therefore, this removal fixes the the warnings related to things that don't work anyway.
To give us a bit more comfort going forward with this removal, this is what the output of locale
currently looks like (i.e. before this removal):
LANG=en_US.utf8
LC_CTYPE="en_US.utf8"
LC_NUMERIC="en_US.utf8"
LC_TIME="en_US.utf8"
LC_COLLATE="en_US.utf8"
LC_MONETARY="en_US.utf8"
LC_MESSAGES="en_US.utf8"
LC_PAPER="en_US.utf8"
LC_NAME="en_US.utf8"
LC_ADDRESS="en_US.utf8"
LC_TELEPHONE="en_US.utf8"
LC_MEASUREMENT="en_US.utf8"
LC_IDENTIFICATION="en_US.utf8"
LC_ALL=
As you can see, LC_ALL is empty for the reason mentioned above. Thus, it's no harm.
Co-authored-by: Nurlan Moldomurov <nurlan.moldomurov@percona.com>
PMM-12641
Link to the Feature Build: SUBMODULES-3739
To the reviewer
This PR is another iteration on this ticket, so it's probably not the final one. However, the number of changes is significant, so I prefer to submit it for review now.