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

Publishers: stdout and AMS #178

Merged
merged 11 commits into from
Nov 5, 2019
Merged

Publishers: stdout and AMS #178

merged 11 commits into from
Nov 5, 2019

Conversation

enolfc
Copy link
Contributor

@enolfc enolfc commented Oct 23, 2019

Summary

This PR brings pluggable support for the output. 2 initial publishers are provided: stdout (the one we already had) and ams for pushing messages to ARGO AMS

@enolfc enolfc requested a review from gwarf October 23, 2019 11:27
@enolfc
Copy link
Contributor Author

enolfc commented Oct 23, 2019

AMS support is broken in python3 :(
Could just use requests instead, but need to check the actual API calls needed for that.

Copy link
Member

@gwarf gwarf left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!
Could you please add some documentation before we merge this?
And what is the status of tests?
Last point, should we release a version before this is merged, or should we wait for it?

@enolfc
Copy link
Contributor Author

enolfc commented Oct 23, 2019

Thanks, looks great!
Could you please add some documentation before we merge this?

yes, will do in a subsequent commit

And what is the status of tests?

I'm now testing with the catch-all stuff, will check with AppDB if they are still getting the same information
And some unit tests will be coming also to this repo

Last point, should we release a version before this is merged, or should we wait for it?

I think is better to wait, as I want ONE providers to use this directly

Only working for python 2 :(
@enolfc
Copy link
Contributor Author

enolfc commented Oct 24, 2019

I believe now it's ready, @gwarf take a look when you can

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@gwarf gwarf self-requested a review October 28, 2019 09:47
enolfc and others added 3 commits October 28, 2019 09:47
Co-Authored-By: Baptiste Grenier <baptiste@bapt.name>
Co-Authored-By: Baptiste Grenier <baptiste@bapt.name>
Co-Authored-By: Baptiste Grenier <baptiste@bapt.name>
Copy link
Member

@gwarf gwarf left a comment

Choose a reason for hiding this comment

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

Looks good, a few small typos, maybe some doc strings on all new methods would be nicer, but it's fine with me.

This will allow AMS to get the right topic automatically
@gwarf gwarf merged commit f4a2438 into EGI-Federation:master Nov 5, 2019
@gwarf gwarf mentioned this pull request Nov 6, 2019
@enolfc enolfc deleted the ams branch November 9, 2020 10:44
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.

2 participants