PR#348: Fix case when older composes have Compose.target_dir set to None. - odcs

odcs

#348 Fix case when older composes have Compose.target_dir set to None.

Merged 4 years ago by lsedlar. Opened 4 years ago by jkaluza.

jkaluza/odcs volume-2 into master

Do not set default target_dir in database migration.

Jan Kaluza • 4 years ago

d4390d4

Fix case when older composes have Compose.target_dir set to None.

Jan Kaluza • 4 years ago

fc4c846

server/odcs/server/backend.py

file modified

+1 -1

		`@@ -911,7 +911,7 @@`

		`# Be nice to end user and replace paths to logs or other files with URL`
		`# accessible to the user.`
		`- if compose.target_dir == conf.target_dir:`
		`+ if compose.on_default_target_dir:`
		`state_reason = state_reason.replace(conf.target_dir, conf.target_dir_url)`
		`compose.transition(COMPOSE_STATES["failed"], state_reason)`

server/odcs/server/migrations/versions/812f2745248f_.py

file modified

+1 -2

		`@@ -12,11 +12,10 @@`

		`from alembic import op`
		`import sqlalchemy as sa`
		`- from odcs.server import conf`


		`def upgrade():`
		`- op.add_column('composes', sa.Column('target_dir', sa.String(), nullable=True, default=conf.target_dir))`
		`+ op.add_column('composes', sa.Column('target_dir', sa.String(), nullable=True))`


		`def downgrade():`

server/odcs/server/models.py

file modified

+11 -4

		`@@ -161,7 +161,14 @@`
		`celery_task_id = db.Column(db.String, nullable=True)`
		# Target directory in which the compose is stored. This is `conf.target_dir`
		`# by default.`
		`- target_dir = db.Column(db.String)`
		`+ target_dir = db.Column(db.String, nullable=True)`
		`+`
		`+ @property`
		`+ def on_default_target_dir(self):`
		`+ """`
		+ True if this compose is stored on default `conf.target_dir`.
		`+ """`
		`+ return self.target_dir is None or self.target_dir == conf.target_dir`

		`@classmethod`
		`def create(cls, session, owner, source_type, source, results,`
		`@@ -286,7 +293,7 @@`
		`"""`
		`Returns public URL to compose directory with per-arch repositories.`
		`"""`
		`- if self.target_dir != conf.target_dir:`
		`+ if not self.on_default_target_dir:`
		`return ""`

		`target_dir_url = conf.target_dir_url`
		`@@ -307,7 +314,7 @@`
		`"""`
		`Returns public URL to repofile.`
		`"""`
		`- if self.target_dir != conf.target_dir:`
		`+ if not self.on_default_target_dir:`
		`return ""`

		`target_dir_url = conf.target_dir_url`
		`@@ -338,7 +345,7 @@`
		`if self.results & value:`
		`results.append(name)`

		`- if self.target_dir == conf.target_dir:`
		`+ if self.on_default_target_dir:`
		`target_dir = "default"`
		`else:`
		`inverse_extra_target_dirs = {v: k for k, v in conf.extra_target_dirs.items()}`

server/odcs/server/pungi.py

file modified

+1 -1

		`@@ -511,7 +511,7 @@`
		`continue`
		`errors += error`

		`- if self.compose.target_dir == conf.target_dir:`
		`+ if self.compose.on_default_target_dir:`
		`errors = errors.replace(`
		`conf.target_dir, conf.target_dir_url)`
		`return errors`

no initial comment

lsedlar commented 4 years ago

Right, the default value in migration is not going to fix existing records. I'm wondering if it should be removed. It basically hardcodes the default on database level to whatever it is when migration runs. I don't think we will ever want to change the default, but it would be tricky because of this.

jkaluza commented 4 years ago

Yeah, I will remove it as part of this PR. It's so far just ODCS fedora staging which is deployed with this.

lsedlar commented 4 years ago

No problem, let's iron this out and make a new release once these issues are fixed.

1 new commit added

Do not set default target_dir in database migration.

4 years ago

lsedlar commented 4 years ago

I'm not sure I like modifying the migration after it's been included in a release. I know it's not deployed anywhere, but would it be safer to create a new migration instead?

jkaluza commented 4 years ago

Hm, I will see what I can do, but I'm not sure if it's possible to use sqlalchemy to remove already introduced default somehow without removing the column and adding it again.

lsedlar commented 4 years ago

I'm probably wrong: based on alembic docs it looks like the default does something on Python site, but does not set anything on the database level. It may be harmless in the migration.

However I'm now confused about the target_dir column. In models.py it has no extra arguments, but the migration created it as nullable=True and with the default.

2 new commits added