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

Scenario upload rework #123

Merged
merged 7 commits into from
May 19, 2020
Merged

Scenario upload rework #123

merged 7 commits into from
May 19, 2020

Conversation

DenysGonchar
Copy link
Collaborator

This PR reworks scenario (module) uploading to amoc cluster. Now the PUT HTTP method is used instead of POST, so JSON encoding of erlang code is not required any more.

Please note that there is no support for file uploading using PUT method in the current swagger 2.0 user interface.

REST API documentation will be updated in a subsequent PR.

Copy link
Member

@chrzaszcz chrzaszcz 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 and it seems much more convenient to upload the file now. Just to clarify:

  • Why do we need to change the method from POST to PUT? PUT seems better as file upload is idempotent and it's the default method in curl -T. Was this the reason?
  • I see that there is a new path /upload needed to handle the new content type, right? Why do we need application/json for /scenarios though as there is only GET there now?


post_scenarios_returns_200_and_when_scenario_valid(_Config) ->
Copy link
Member

Choose a reason for hiding this comment

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

we could remove the "and"

echo "Installing scenario: 'dummy_scenario.erl' on node amoc-1 (port ${PORT1})"

SCEN_POST=$( (echo '{"scenario":"dummy_scenario","module_source":'; echo ${SCEN_SOURCE} ; echo '}' ) | curl \
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name it SCEN_PUT just to avoid confusion.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

The old way was painful to use, the simple text/plain approach is much more convenient. When it comes to commits history, I think they could be squashed to 3:

  1. Rebar3 upgrade
  2. The change the PR is about
  3. Erlang/OTP update

I hope the subsequent PR with documentation changes comes soon :)

Also, I noticed the PR does some extra change related to upgrading of rebar3 and used Erlang/OTP versions, ideally, it'd good to have that in a separate PR but I'm not going to reject this one because of that.

@michalwski
Copy link
Contributor

Oh, and I just noticed, that dialyzer returns some errors after these changes.

@DenysGonchar
Copy link
Collaborator Author

Why do we need to change the method from POST to PUT? PUT seems better as file upload is idempotent and it's the default method in curl -T. Was this the reason?

yes, PUT is more appropriate for these purposes.

Copy link
Contributor

@michalwski michalwski left a comment

Choose a reason for hiding this comment

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

I think it can be merged. Commits history could still be improved, let me know if you want to do it or don't care that much.

@michalwski michalwski merged commit b27aa48 into master May 19, 2020
@michalwski michalwski deleted the scenario-upload-rework branch May 19, 2020 08:21
@chrzaszcz
Copy link
Member

I was still waiting for the answer for this question:

I see that there is a new path /upload needed to handle the new content type, right? Why do we need application/json for /scenarios though as there is only GET there now?

@DenysGonchar
Copy link
Collaborator Author

Because that is declaration of response type for this GET request.

@chrzaszcz
Copy link
Member

Because that is declaration of response type for this GET request.

I still don't think I got the answer. Let me rephrase the question then:

Why use a separate path /uploads instead of /scenarios as before? It is not wrong, I just would like to know your exact motivation.

@DenysGonchar
Copy link
Collaborator Author

I prefer to have different paths for different functionality. it's easier to keep codebase clean, and easier to explain in docs.

@michalwski
Copy link
Contributor

michalwski commented May 19, 2020

It just came to my mind that, maybe path like /scenarios/upload would be more self-explanatory here.

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

3 participants