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

Store StagingWorkflow in the backend #6206

Conversation

DavidKang
Copy link
Contributor

StagingWorkflow is stored in the backend under _project directory with
the name _staging_workflow. In this special file, we store a minimal
info of the staging_workflow.

@DavidKang DavidKang added the Frontend Things related to the OBS RoR app label Nov 8, 2018
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch 2 times, most recently from 4477ed5 to b7bb289 Compare November 8, 2018 15:07
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch 2 times, most recently from 1a82493 to 9fd9e36 Compare November 9, 2018 09:58
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch 3 times, most recently from 0f90425 to fe1d1a3 Compare November 9, 2018 15:01
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch from fe1d1a3 to fdc7547 Compare November 12, 2018 16:38
@adrianschroeter
Copy link
Member

You should also implement the read and update from the file to the database in the same commit/pull request if you use a source file here. Otherwise data can run out of sync easily.

@coolo
Copy link
Member

coolo commented Nov 13, 2018

writing to that file outside of the model should be plainly forbidden IMO.

@coolo
Copy link
Member

coolo commented Nov 13, 2018

how about?

diff --git a/src/api/config/routes.rb b/src/api/config/routes.rb
index bb4204d7f..a49cc73c0 100644
--- a/src/api/config/routes.rb
+++ b/src/api/config/routes.rb
@@ -768,6 +768,7 @@ OBSApi::Application.routes.draw do
 
     get 'source/:project/:package/:filename' => :get_file, constraints: cons, defaults: { format: 'xml' }
     delete 'source/:project/:package/:filename' => :delete_file, constraints: cons
+    put 'source/:project/_project/_staging_workflow' => :just_dont, constraints: cons
     put 'source/:project/:package/:filename' => :update_file, constraints: cons

@adrianschroeter
Copy link
Member

That would be just one out of many ways to work on it. And why should it be forbidden, if you add it here it is obvious that it is also the API to create/modify it, no?

@coolo
Copy link
Member

coolo commented Nov 13, 2018

But it's not. Creating a staging project requires more than adding it to a xml file.

@coolo
Copy link
Member

coolo commented Nov 13, 2018

So possibly the _staging_workflow shouldn't be a file in _project after all - where else to store it to make it obvious that it can't be edited at free will?

@hennevogel
Copy link
Member

This is how we forbid other underscore files. An API for creating/changing this is not what we are going to do now.

src/api/app/models/project.rb Outdated Show resolved Hide resolved
src/api/spec/models/staging_workflow_spec.rb Outdated Show resolved Hide resolved
@adrianschroeter
Copy link
Member

@hennevogel this is on package name level and not on file level.

The api it needs also be writeable anyway in the end, it is bad idea to add cruel hacks to workaround some situations (and for sure not all). It shows that the design should have been documented first before hacking ....

@coolo
Copy link
Member

coolo commented Nov 13, 2018

does if @file == '_attribute' ring any bells, @adrianschroeter ?

@coolo
Copy link
Member

coolo commented Nov 13, 2018

using a file to create staging projects is a bad API anyway. So let's talk about an API to modify staging workflows independent of _project

@adrianschroeter
Copy link
Member

adrianschroeter commented Nov 13, 2018

_attribute is a very special case, because it can not be bind to the package model container of "_project" (since it is project model). reading works different there anyway. But this not true for a regular source file.

dunno if a file in _project makes sense for this, but it must be configurable via api anyway and there should not be two ways for same content. So, yes, better define first how to create/modify/delete it via api first before defining the place.

@hennevogel
Copy link
Member

So let's talk about an API to modify staging workflows independent of _project

We will do that for sure, I don't like having UI or API only features either. But not now, in a later iteration.

@adrianschroeter
Copy link
Member

@hennevogel fine if you don't want to do it now, but then don't store this file either now. It only cries for problems, stuff will be out of sync, people may use it already because they can see it....

I was only against to it partly and creating all these problems.

@hennevogel
Copy link
Member

but then don't store this file either now

We can also just "forbid" writing to this file like we do for _attribute for now.

@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch from fdc7547 to d1fe4d5 Compare November 13, 2018 14:17
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch from d1fe4d5 to 297b378 Compare November 13, 2018 15:50
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch 3 times, most recently from c1ec051 to 00f0731 Compare November 15, 2018 11:24
StagingWorkflow is stored in the backend under `_project` directory with
the name `_staging_workflow`. In this special file, we store a minimal
info of the staging_workflow.
@DavidKang DavidKang force-pushed the staging_workflow/sprint50/store-staging-workflow branch from 00f0731 to a621342 Compare November 15, 2018 11:29
@DavidKang
Copy link
Contributor Author

@hennevogel and @bgeuken could you review it again?

@hennevogel
Copy link
Member

code* and hakiri hang. Coverage is fine, see coveralls and there are no security warnings on hakiri for this commit. Merging...

@hennevogel hennevogel merged commit 3177dbb into openSUSE:staging-workflow Nov 15, 2018
@DavidKang DavidKang deleted the staging_workflow/sprint50/store-staging-workflow branch November 15, 2018 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend Things related to the OBS RoR app
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants