Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented Jul 12, 2018

This pull request updates all the runner packages to they read their version from the runner package local __init__.py file.

Sadly we can't go with approach in #4211 due to a cyclic dependency issue (which right now only exists for orchestra runner, but once we move to dynamic stevedore loading will exist for every runner package if we make runners depend directly on st2common) - #4211 (comment).

This way it's consistent with st2client.

Second part of this change is updating st2cd release workflow so it also updates version for all the runner packages (in the same place where we set version for st2common and st2client).

__init__.py file.

This way it's consistent with st2client and we can set those versions
during the release process.
from action_chain_runner import __version__

BASE_DIR = os.path.dirname(os.path.abspath(__file__))
REQUIREMENTS_FILE = os.path.join(BASE_DIR, 'requirements.txt')
Copy link
Member

@arm4b arm4b Jul 13, 2018

Choose a reason for hiding this comment

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

Since import doesn't work, can the version be read from the ../../../../../st2common/__init__.py via similar "file" approach?
How worse or more hacky that could be, comparing to hardcoding same version number in all the runners?

To me, ideally to change the version in one single place.

Copy link
Member Author

Choose a reason for hiding this comment

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

This would kinda break the whole package approach - it would rely on st2common to be in a specific path and always available with a runner package.

And it's not really hard coding - it's being updated via release workflows. It's exactly the same approach we follow for st2common and st2client.

And this also allows us to separately version and release runner packages in the future (if we ever need to do that).

Copy link
Member

Choose a reason for hiding this comment

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

I understand the purpose of st2common and st2client being versioned separately, but don't get the reason (yet) for runners which are part of the core and are in st2 repo code.

Any future plans to do extract these runners from the core and distribute separately?

Copy link
Member Author

Choose a reason for hiding this comment

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

No plans for that right, but with package approach we do get more flexibility which allows us to do things like that in the future.

I believe @bigmstone did talk something about moving runners to standalone repos, but I personally don't want to do that any time soon yet (too much overhead).

Copy link
Member Author

Choose a reason for hiding this comment

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

And also, with those changes they are not really versioned separately / interdependently - as mentioned above, it follows the same approach as other components and we bump / set the version during the release process.

Copy link
Member

Choose a reason for hiding this comment

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

I just feel it's a bit of mess to change the version in 10 files with the same number without having plans to extract these parts from the core repo.
If there are no other options to brainstorm, - let it be 👍

Would just like @bigmstone or @m4dcoder to jump in and review the PR, as someone more familiar with st2 core and plans in context of runners.

@arm4b arm4b requested review from bigmstone and m4dcoder July 13, 2018 11:14
Copy link
Contributor

@bigmstone bigmstone left a comment

Choose a reason for hiding this comment

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

I get @armab concerns, but I'm fine with this as a stepping stone towards something else in the future.

One question I do have (I'm too fuzzy on the build details at the moment), do we actually need all these versions to match? I believe build process will require them to be correct version but can't remember. That is probably what should be updated, and then we just version these runners independent from ST2 core code.

@Kami
Copy link
Member Author

Kami commented Jul 17, 2018

@bigmstone Thanks.

Versions don't need to match, but right now everything is coupled so there is no reason for versions not to match :)

The change you did for orchestra runner doesn't specify a version. Going forward with rest of the runners, we could handle it in the same way as we handle st2client and inject runner dependencies with a version which matches StackStorm version.

Since runner packages are not published on PyPi (unlike st2client), this is not strictly needed, but it shouldn't hurt either.

@arm4b
Copy link
Member

arm4b commented Jul 17, 2018

I like @bigmstone point that, in fact, version for runners don't need to match and question why are we incrementing them, practically. That definitely makes more sense to me, - incrementing version when the content really changed, not just for the sake of versioning and automating the automation puzzle.

Anyways, not blocking. Hopefully it will come up with something good in the future 🤷‍♂️

@Kami Kami merged commit 5bd8c1f into master Jul 17, 2018
@Kami Kami deleted the runner_versions_v2 branch July 17, 2018 18:35
Kami added a commit that referenced this pull request Jul 17, 2018
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.

4 participants