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

Add retry logic around Trilio CRUD commands #770

Merged
merged 1 commit into from Jan 11, 2023

Conversation

gnuoy
Copy link
Contributor

@gnuoy gnuoy commented May 24, 2022

No description provided.

Copy link
Contributor

@ajkavanagh ajkavanagh left a comment

Choose a reason for hiding this comment

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

LGTM - just some minor comments on things I thought of whilst reviewing.

@tenacity.retry(
retry=tenacity.retry_if_exception_type(zaza_model.CommandRunFailed),
wait=tenacity.wait_fixed(10), # interval between retries
stop=tenacity.stop_after_attempt(5)) # retry 10 times
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment and param don't match? e.g. 10 times vs 5.

:rtype: string
:raises: model.CommandRunFailed
"""
logging.info("Running {}".format(remote_cmd))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's entirely optional, but I tend to use the %s form of interpolating into strings in logging commands as it's slightly more efficient as if the log doesn't happen (e.g. no info logs logged due to settings), it doesn't actually do the interpolation ... i.e. it makes it a bit lazy. The .format() always runs regardless of whether the log happens or not. I only found this out this year, and I've been doing it ever since!

openstack-mirroring pushed a commit to openstack/charm-trilio-dm-api that referenced this pull request Jun 22, 2022
Due to a build problem with the reactive plugin, this change falls back
on overriding the steps and doing a manual build, but it also ensures
the CI system builds the charm using charmcraft.  Changes:

- add a build-requirements.txt
- modify charmcraft.yaml
- modify osci.yaml
    -> indicate build with charmcraft
- modify tox.ini
    -> tox -e build does charmcraft build/rename
    -> tox -e build-reactive does the reactive build
- modify bundles to use the <charm>.charm artifact in tests.
  and fix deprecation warning re: prefix
- tox inception to enable tox -e func-test in the CI
- Pin everything in wheelhouse.txt for bionic and focal support.
  This will need updating when the charm channels are created.

func-test-pr: openstack-charmers/zaza-openstack-tests#770
Change-Id: Ie0f9e4a80b81eaeb91ce11b84a54c7ecc95cdff2
openstack-mirroring pushed a commit to openstack/openstack that referenced this pull request Jun 22, 2022
* Update charm-trilio-dm-api from branch 'master'
  to fa6223fa3afc98f4033e58c7bc16dbc96516e765
  - Update to build using charmcraft
    
    Due to a build problem with the reactive plugin, this change falls back
    on overriding the steps and doing a manual build, but it also ensures
    the CI system builds the charm using charmcraft.  Changes:
    
    - add a build-requirements.txt
    - modify charmcraft.yaml
    - modify osci.yaml
        -> indicate build with charmcraft
    - modify tox.ini
        -> tox -e build does charmcraft build/rename
        -> tox -e build-reactive does the reactive build
    - modify bundles to use the <charm>.charm artifact in tests.
      and fix deprecation warning re: prefix
    - tox inception to enable tox -e func-test in the CI
    - Pin everything in wheelhouse.txt for bionic and focal support.
      This will need updating when the charm channels are created.
    
    func-test-pr: openstack-charmers/zaza-openstack-tests#770
    Change-Id: Ie0f9e4a80b81eaeb91ce11b84a54c7ecc95cdff2
@ajkavanagh ajkavanagh merged commit bf2273c into openstack-charmers:master Jan 11, 2023
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.

None yet

2 participants