AMQ adapter version w/o endpoints for CleverSwarm integration #3
No reviewers
Labels
No labels
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Support
Type
Task
Type
Testing
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: clevermicro/amq-adapter-python#3
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/#1"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Ref epic: CleverSwarm Onboarding #58
this implements #1
requested review from @abed.alrahman and @hurui200320
assigned to @stanislav.hejny
In this case, we can accept both str and bytes-like object for messages. I think we can mark the type of
message
asstr | bytes
(I'd prefer immutable typebytes
instead of mutablebytearray
, and other bytes-like object types don't even make sense here). Although it doesn't impact anything during the runtime, it does help on static analysis (if we have one) when someone tries to give an int number, since I assume thebase64.b64encode
doesn't accept int objects.Also something like Javadoc (but for python, I don't know what it's called) might be helpful to tell the user (presumably Luis) which type you can pass in.
If this adaptor is specifically made for CleverSwarm, should it be part of the CleverSwarm code base, instead of living in this repo as a public library? It's ok if it's temporarily living there for the integration since it's time-limited, but long term wise I think it's better moved to CleverSwarm code base.
It's not good to hardcode such logic in the code. Generally I would recommend using a cmdline option or an env variable for this. For example,
FOO_CONFIG_PATH
(replace theFOO
with library name, using it as a namespace). If the env is not provided or the value is not pointing to a valid file, then you can fall back on default values like~/.config/foo/application.properties
(using the home folder of the user, which should be available on both linux and windows).I suggest we put the
UNKNOWN
at the beginning, so that in the future we don't have cases likeREQUEST_FOO=1
,UNKNOWN=2
andREQUEST_BAR=3
when adding new items. This is more like a style thing.These files are no longer needed since they don't work with docker swarm. I would suggest we get rid of those files before merging. But if you need them for testing and debugging, feel free to ignore this comment.
Thank you for your work. I'm not very familiar with python projects, so I might not be able to catch some specific bugs. Currently I did a line-by-line review and don't see any problems. All my comments are more like a suggestion, so feel free to ignore them if you have a reason to do so or I'm wrong.
If you need more professional opinion on the code, I suggest you ask Jeff or people from other teams who know python and related libraries.
requested changes
U'r right, this is a shortcut, because the final version is still dependent on final integration effort. The problem is that I need a path to file from CWD (or absolute path) and I don't know what it would be after integration. Even the cmd-line parameter has to be decided with CleveSwarm. this should be perhaps marked as TODO or better documented
I will change that in later version (v0.2), because in Python, enum ordinals start at 1, while in Java enum ordinals start at 0. I need to adjust the same enums on Java side so that the ordinal values are matching (at this moment I add 1 when decoding the message produced by Java based adapter).
I perhaps don't need them all, it was just the docker compose won't work w/o it. But indeed those are only for the local testing purposes.
Ok, in that case you can add a TODO in the code and mark this thread as resolved.
added 5 commits
635b1003
- Initial commit0bbd8e29
- Added Apache OSS license6f35b5e4
- Initial commitd2b1b59c
- Converted AMQPAdapeter Java code to Python286a4986
- AMQ adapter version w/o endpints for CleverSwarm integrationCompare with previous version
changed this line in version 4 of the diff
added 1 commit
eb26eec7
- CleverSwarm API invocation exampleCompare with previous version
Changed to be bytes only
This entire python code is to be included to CleverSwarm, as library, perhaps, but we start by giving CS everything. Still need to work out how to make it more generic
resolved all threads
changed this line in version 5 of the diff
added 1 commit
1f904072
- feat/#1 - refactor code to allow make it as library and to separate the AMQ...Compare with previous version
added 1 commit
73c962dc
- Add CI/CD pipeline, adjust for integration with CleverSwarm and other python servicesCompare with previous version
enabled automatic add to merge train when checks pass
aborted automatic add to merge train because the source branch was updated. Learn more.
added 1 commit
1b042196
- feat/#1 - add the code coverage run to the pipelineCompare with previous version
enabled automatic add to merge train when checks pass
bypassed reviews on this merge request
aborted automatic add to merge train because the source branch was updated. Learn more.
added 1 commit
a633ecef
- Update the Ci/CD pipline to publish AMQ as libraryCompare with previous version
added 1 commit
30e046b2
- Update the Ci/CD pipline to publish AMQ as libraryCompare with previous version
requested review from @hurui200320
mentioned in merge request !4
approved this merge request
approved this merge request
added 1 commit
e861bf0f
- Remove references to cleverswarm and fix packagingCompare with previous version
reset approvals from @abed.alrahman and @hurui200320 by pushing to the branch
added 1 commit
dc884c65
- Bump-up project version to enable publish to PyPICompare with previous version
added 1 commit
aaf1c77e
- Fix cleverswarm dependencies identified in testingCompare with previous version
added 1 commit
aaa2895c
- remove hardcoded config file pathCompare with previous version
added 1 commit
d9af0d09
- Bump-up project version to enable publish to PyPICompare with previous version
approved this merge request
approved this merge request
started a merge train
removed this merge request from the merge train because The resulting pipeline would have been empty. Review the rules configuration for the relevant jobs.
mentioned in commit
e8f4abdfaf