Skip to content
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

Upgrades python version for cqlsh #4709

Merged
merged 2 commits into from
Jan 24, 2022
Merged

Conversation

davidporter-id-au
Copy link
Contributor

@davidporter-id-au davidporter-id-au commented Jan 24, 2022

What changed?

  • Added python3 dep via APK
  • changed CQLSH installation to use pip3

Why?
cqlsh is broken

How did you test it?

➜  cadence docker images
REPOSITORY                   TAG                 IMAGE ID       CREATED              SIZE
ubercadence/cadence-canary   master              981cc1952c2c   About a minute ago   123MB
ubercadence/cadence-bench    master              1cc084161128   About a minute ago   117MB
ubercadence/cli              master              65b60f971e33   About a minute ago   94.8MB
ubercadence/server           master-auto-setup   7063ae11fc67   About a minute ago   371MB
ubercadence/server           master              25648ebb01d0   About a minute ago   277MB
➜  cadence docker run -ti  7063ae11fc67  cqlsh
Connection error: ('Unable to connect to any servers', {'127.0.0.1:9042': ConnectionRefusedError(111, "Tried connecting to [('127.0.0.1', 9042)]. Last error: Connection refused")})

Potential risks

Release notes

Documentation Changes

Dockerfile Outdated

COPY docker/start.sh /start.sh

# test installed applications
RUN cqlsh --version
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after testing this wouldn't have actually caught the cqlsh issue, which fails at runtime only when executing, but it would have caught other issues, such as failure to correctly install to path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe pip3 install cqlsh && cqlsh --version so we don't make another layer?
but probably not a bad idea, since the install is far from a guarantee with python.

I suppose it's somewhat odd that there's no support for testing builds in dockerfiles 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think normally you treat them as build artifacts and subject them to e2e testing as you would any other deployable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, though in-dockerfile would let you test things as you build rather than only at the end. e.g. for from scratch things, you could test in a build-and-debug-friendly env before you copy the binaries to a minimal image.

Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great, thanks! simple fixes are always nice :)
and it's good to leave py2. it's definitely time to do so.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2e7767c2-235d-4c24-a4ff-f23a937c339e

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 74 unchanged lines in 14 files lost coverage.
  • Overall coverage decreased (-0.02%) to 56.93%

Files with Coverage Reduction New Missed Lines %
client/history/client.go 2 39.33%
client/history/metricClient.go 2 46.37%
common/task/weightedRoundRobinTaskScheduler.go 2 88.6%
common/types/mapper/thrift/shared.go 2 63.25%
service/history/handler.go 2 48.56%
service/matching/matcher.go 2 91.46%
common/cache/lru.go 3 90.73%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 3 55.49%
service/history/task/fetcher.go 3 86.67%
service/frontend/workflowHandler.go 4 59.4%
Totals Coverage Status
Change from base Build 44b36639-36d5-40b3-9df5-aa902204e7ff: -0.02%
Covered Lines: 82904
Relevant Lines: 145625

💛 - Coveralls

@davidporter-id-au davidporter-id-au merged commit 99ace7a into master Jan 24, 2022
@davidporter-id-au davidporter-id-au deleted the bugfix/fix-cqlsh branch January 24, 2022 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants