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

[api] fix trigger route behaviour #15119

Closed

Conversation

adrianschroeter
Copy link
Member

@adrianschroeter adrianschroeter commented Oct 26, 2023

  • fail when wrong operation type is used (instead of executing the token operation)
  • allow to use /trigger without specifing the operation type (is defined by the token)
  • verify given project/package parameter with possible token package
  • fix triggering rebuilds and releases of entire projects
  • improve test coverage and api docu
    • A preview of the changes in the api documentation can be seen with the review-app here.

@github-actions github-actions bot added Documentation 📖 Things regarding our documentation Frontend Things related to the OBS RoR app labels Oct 26, 2023
Copy link
Member

@eduardoj eduardoj left a comment

Choose a reason for hiding this comment

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

This would require changes in the current API documentation...

@dirkmueller
Copy link
Member

@eduardoj are you saying src/api/public/apidocs/paths/trigger_operation.yaml needs to be modified?

@eduardoj
Copy link
Member

@eduardoj are you saying src/api/public/apidocs/paths/trigger_operation.yaml needs to be modified?

@dirkmueller, yes. Either we modify that file or we split that file in three:

  • src/api/public/apidocs/paths/trigger_rebuild.yaml
  • src/api/public/apidocs/paths/trigger_release.yaml
  • src/api/public/apidocs/paths/trigger_runservice.yaml

Not only that, a new endpoint has been introduced: POST /trigger. This should also be documented, most likely in a new file named src/api/public/apidocs/paths/trigger.yaml.

@adrianschroeter
Copy link
Member Author

I just renamed it now, given that the new endpoint is just a different name without the so far missing operation type check.

@eduardoj eduardoj added the review-app Apply this label if you want a review app started label Oct 27, 2023
@obs-bot
Copy link
Collaborator

obs-bot commented Oct 27, 2023

@eduardoj
Copy link
Member

I just renamed it now, given that the new endpoint is just a different name without the so far missing operation type check.

@adrianschroeter, I see you added changes to the current documentation. Thanks.

But renaming the src/api/public/apidocs/paths/trigger_operation.yaml to the src/api/public/apidocs/paths/trigger.yaml file makes Swagger-UI not able to find that file. See the OBS-v2.10.50.yaml file:

/trigger/{operation}:
$ref: 'paths/trigger_operation.yaml'

I can imagine solving this issue using one of these two options:

  1. Renaming back the file to src/api/public/apidocs/paths/trigger_operation.yaml. This is fine because we define the {operation} path parameter as "not required".
  2. Keep the file renaming. We should change the reference to the file from $ref: 'paths/trigger_operation.yaml' to $ref: 'paths/trigger.yaml'

I prefer the first option, renaming back the file to src/api/public/apidocs/paths/trigger_operation.yaml.. I recognize this contradicts with my first comment: Not only that, a new endpoint has been introduced: POST /trigger. This should also be documented, most likely in a new file named src/api/public/apidocs/paths/trigger.yaml.. This solution you propose convinces my. Sorry if my first comment was misleading.

@adrianschroeter
Copy link
Member Author

okay, will rename the docu file back to origin name.

- fail when wrong operation type is used (instead of executing the token
  operation)
- allow to use /trigger without specifing the operation type (is defined
  by the token)
- verify given project/package parameter with possible token package
- fix triggering rebuilds and releases of entire projects
- improve test coverage and api docu
@eduardoj
Copy link
Member

I added a commit to fix the RuboCop offenses. Let's see if the previous changes make some other fail.

Copy link
Member

@eduardoj eduardoj left a comment

Choose a reason for hiding this comment

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

The following specs must be adapted:

  • spec/controllers/person/token_controller_spec.rb
  • spec/controllers/trigger_controller_spec.rb
  • spec/controllers/webui/users/token_triggers_controller_spec.rb

@adrianschroeter
Copy link
Member Author

@eduardoj can you update the specs please? I do not have currently an environment to run these.

@eduardoj
Copy link
Member

eduardoj commented Nov 15, 2023

I added a commit to get rid of the codeclimate complaint.

I'll try to adapt the failing tests:

  • spec/controllers/person/token_controller_spec.rb
  • spec/controllers/trigger_controller_spec.rb
  • spec/controllers/webui/users/token_triggers_controller_spec.rb
  • spec/controllers/trigger_workflow_controller_spec.rb

@eduardoj eduardoj force-pushed the fix_trigger_operations branch 2 times, most recently from 7f64c9c to 29fb3ae Compare November 17, 2023 15:31
@github-actions github-actions bot added Backend Things regarding the OBS backend Test Suite / CI 💉 Things related to our tests/CI labels Nov 17, 2023
@github-actions github-actions bot removed Backend Things regarding the OBS backend Test Suite / CI 💉 Things related to our tests/CI labels Nov 17, 2023
@hennevogel
Copy link
Member

Superseded by #15261

@hennevogel hennevogel closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📖 Things regarding our documentation Frontend Things related to the OBS RoR app review-app Apply this label if you want a review app started
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants