Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-25 Thread via GitHub


jedcunningham closed pull request #38876: Fix trigger_kwargs 
encryption/decryption on db migration
URL: https://github.com/apache/airflow/pull/38876


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-25 Thread via GitHub


jedcunningham commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2078016764

   Closing this as #39246 has been merged. Thanks for kicking off fixing this 
bug @hussein-awala!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


jedcunningham commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2076404146

   I've opened #39246 as an alternative solution to this.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


jedcunningham commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2076305345

   Currently downgrade also doesn't work when you have triggers - alembic tries 
to switch the column back to json before decrypting happens.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


dstandish commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2076067502

   > > Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?
   > 
   > I don't think so. In my point of view if we wanted to yank then we should 
have merge this quickly and cut 2.9.1 immidiately as critical bug fix. I asked 
about this when issue was discoved but others thought we should wait for more 
more bug fixes as this was not categorized as critical.
   
   To me, I’m not sure whether it meets the criteria of critical, but it’s 
pretty bad. If the user is using home chart, then it will run the migration pot 
a lot, and if there are triggers in flight, this could happen and sorta bork 
the cluster. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


jedcunningham commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2076010902

   Just confirmed my suspicions, it doesn't work for offline migrations:
   
   ```
   [2024-04-24T23:08:17.337+] {triggerer_job_runner.py:341} ERROR - 
Exception when executing TriggererJobRunner._run_trigger_loop
   Traceback (most recent call last):   
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py",
 line 339, in _execute
   self._run_trigger_loop() 
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py",
 line 362, in _run_trigger_loop   
   self.load_triggers() 
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py",
 line 377, in load_triggers   
   self.trigger_runner.update_triggers(set(ids))
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/jobs/triggerer_job_runner.py",
 line 676, in update_triggers 
   new_trigger_instance = trigger_class(**new_trigger_orm.kwargs)   
 
  ^^
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/models/trigger.py", 
line 93, in kwargs  
   return self._decrypt_kwargs(self.encrypted_kwargs)   
 
  ^^^   
 
 File 
"/home/airflow/.local/lib/python3.12/site-packages/airflow/models/trigger.py", 
line 119, in _decrypt_kwargs
   decrypted_kwargs = 
json.loads(get_fernet().decrypt(encrypted_kwargs.encode("utf-8")).decode("utf-8"))
 
 
^^  

 File 
"/home/airflow/.local/lib/python3.12/site-packages/cryptography/fernet.py", 
line 211, in decrypt   
   raise InvalidToken   
 
   cryptography.fernet.InvalidToken 
 
   ```
   
   This was all that was spit out for that migration:
   
   ```
   -- Running upgrade ee1467d4aa35 -> 1949afb29106
   
   ALTER TABLE trigger ALTER COLUMN kwargs TYPE TEXT;
   
   UPDATE alembic_version SET version_num='1949afb29106' WHERE 
alembic_version.version_num = 'ee1467d4aa35';
   ```


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


jedcunningham commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2075998782

   Ignoring that the conditional is wrong, I don't follow how this migration 
approach works for offline migration in the first place. Am I missing 
functionality somewhere that solves that scenario?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


eladkal commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2075624714

   > Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?
   
   I don't think so.
   In my point of view if we wanted to yank then we should have merge this 
quickly and cut 2.9.1 immidiately as critical bug fix. I asked about this when 
issue was discoved but others thought we should wait for more more bug fixes as 
this was not categorized as critical.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


dstandish commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2075334734

   Should we consider yanking 2.9.0 once 2.9.1 is out with this fix?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


Taragolis commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1577540292


##
tests/utils/test_db.py:
##
@@ -143,23 +143,29 @@ def test_offline_upgrade_wrong_order(self, from_revision, 
to_revision):
 ],
 )
 def test_offline_upgrade_revision_nothing(self, from_revision, 
to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade"):
-with redirect_stdout(StringIO()) as temp_stdout:
-upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
-stdout = temp_stdout.getvalue()
-assert "nothing to do" in stdout
-
+with mock.patch("alembic.command.upgrade"):
+with redirect_stdout(StringIO()) as temp_stdout:
+upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
+stdout = temp_stdout.getvalue()
+assert "nothing to do" in stdout
+
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",
+)
 @pytest.mark.parametrize(
 "from_revision, to_revision",
 [("90d1635d7b86", "54bebd308c5f"), ("e959f08ac86c", "587bdf053233")],
 )
 def test_offline_upgrade_revision(self, from_revision, to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
-upgradedb(from_revision=from_revision, 
to_revision=to_revision, show_sql_only=True)
+with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
+upgradedb(from_revision=from_revision, to_revision=to_revision, 
show_sql_only=True)
 mock_alembic_upgrade.assert_called_once_with(mock.ANY, 
f"{from_revision}:{to_revision}", sql=True)
 
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",
+)

Review Comment:
   Maybe mark it for supported backend?
   
   ```suggestion
   @pytest.mark.backend("postgres", "mysql")
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


uranusjr commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1577341418


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _from_revision
+trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
+if (
+_from_revision != trigger_kwargs_encryption_revision
+and _revision_greater(config, trigger_kwargs_encryption_revision, 
_from_revision)
+and _revision_greater(config, current_version, 
trigger_kwargs_encryption_revision)
 ):
+# _from_revision < trigger_kwargs_encryption_version <= current_version

Review Comment:
   Is there confusion between `from_revision` I wonder? Not sure which is 
intended here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-24 Thread via GitHub


uranusjr commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1577334918


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _from_revision
+trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
+if (
+_from_revision != trigger_kwargs_encryption_revision
+and _revision_greater(config, trigger_kwargs_encryption_revision, 
_from_revision)
+and _revision_greater(config, current_version, 
trigger_kwargs_encryption_revision)
 ):
+# _from_revision < trigger_kwargs_encryption_version <= current_version

Review Comment:
   Same here, this comment looks quite wrong here.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-23 Thread via GitHub


Lee-W commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1577081620


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _from_revision
+trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
+if (
+_from_revision != trigger_kwargs_encryption_revision
+and _revision_greater(config, trigger_kwargs_encryption_revision, 
_from_revision)
+and _revision_greater(config, current_version, 
trigger_kwargs_encryption_revision)
 ):
+# _from_revision < trigger_kwargs_encryption_version <= current_version

Review Comment:
   I'm a bit confused here. If `current_vesrion = _from_revision`, then when 
will `_from_revision < trigger_kwargs_encryption_version <= current_version` 
happen?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-17 Thread via GitHub


hussein-awala commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1569650413


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)

Review Comment:
   ```suggestion
   current_version = _from_revision
   ```



##
tests/utils/test_db.py:
##
@@ -143,23 +143,29 @@ def test_offline_upgrade_wrong_order(self, from_revision, 
to_revision):
 ],
 )
 def test_offline_upgrade_revision_nothing(self, from_revision, 
to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade"):
-with redirect_stdout(StringIO()) as temp_stdout:
-upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
-stdout = temp_stdout.getvalue()
-assert "nothing to do" in stdout
-
+with mock.patch("alembic.command.upgrade"):
+with redirect_stdout(StringIO()) as temp_stdout:
+upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
+stdout = temp_stdout.getvalue()
+assert "nothing to do" in stdout
+
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",
+)
 @pytest.mark.parametrize(
 "from_revision, to_revision",
 [("90d1635d7b86", "54bebd308c5f"), ("e959f08ac86c", "587bdf053233")],
 )
 def test_offline_upgrade_revision(self, from_revision, to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
-upgradedb(from_revision=from_revision, 
to_revision=to_revision, show_sql_only=True)
+with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
+upgradedb(from_revision=from_revision, to_revision=to_revision, 
show_sql_only=True)
 mock_alembic_upgrade.assert_called_once_with(mock.ANY, 
f"{from_revision}:{to_revision}", sql=True)
 
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",

Review Comment:
   The tests are not valid for `SQLite` , they should be skipped because of 
https://github.com/apache/airflow/blob/1769ed0b65cae5e7c8b373cbb941eecd051e/airflow/utils/db.py#L1563
   But before `_get_current_revision` was not used if `show_sql_only=True`.
   
   I can refactor it if needed to revert the tests



##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)
+trigger_kwargs_encryption_version = _REVISION_HEADS_MAP["2.9.0"]
+if (
+_from_revision != trigger_kwargs_encryption_version
+and _revision_greater(config, trigger_kwargs_encryption_version, 
_from_revision)
+and _revision_greater(config, current_version, 
trigger_kwargs_encryption_version)

Review Comment:
   ```suggestion
   if (
   _from_revision != trigger_kwargs_encryption_revision
   and _revision_greater(config, trigger_kwargs_encryption_revision, 
_from_revision)
   and _revision_greater(config, current_version, 
trigger_kwargs_encryption_revision)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-17 Thread via GitHub


hussein-awala commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1569650144


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)

Review Comment:
   this was my intention for this refactor 臘‍♂️ 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-16 Thread via GitHub


ephraimbuddy commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1566875191


##
tests/utils/test_db.py:
##
@@ -143,23 +143,29 @@ def test_offline_upgrade_wrong_order(self, from_revision, 
to_revision):
 ],
 )
 def test_offline_upgrade_revision_nothing(self, from_revision, 
to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade"):
-with redirect_stdout(StringIO()) as temp_stdout:
-upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
-stdout = temp_stdout.getvalue()
-assert "nothing to do" in stdout
-
+with mock.patch("alembic.command.upgrade"):
+with redirect_stdout(StringIO()) as temp_stdout:
+upgradedb(to_revision=to_revision, 
from_revision=from_revision, show_sql_only=True)
+stdout = temp_stdout.getvalue()
+assert "nothing to do" in stdout
+
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",
+)
 @pytest.mark.parametrize(
 "from_revision, to_revision",
 [("90d1635d7b86", "54bebd308c5f"), ("e959f08ac86c", "587bdf053233")],
 )
 def test_offline_upgrade_revision(self, from_revision, to_revision):
-with mock.patch("airflow.utils.db.settings.engine.dialect"):
-with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
-upgradedb(from_revision=from_revision, 
to_revision=to_revision, show_sql_only=True)
+with mock.patch("alembic.command.upgrade") as mock_alembic_upgrade:
+upgradedb(from_revision=from_revision, to_revision=to_revision, 
show_sql_only=True)
 mock_alembic_upgrade.assert_called_once_with(mock.ANY, 
f"{from_revision}:{to_revision}", sql=True)
 
+@pytest.mark.skipif(
+conf.get_mandatory_value("database", 
"sql_alchemy_conn").lower().startswith("sqlite"),
+reason="Offline migration not supported for SQLite.",

Review Comment:
   Why do we now have this?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-16 Thread via GitHub


ephraimbuddy commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1566866008


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)
+trigger_kwargs_encryption_version = _REVISION_HEADS_MAP["2.9.0"]

Review Comment:
   ```suggestion
   trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
   ```
   I think we should check with revision as well as version



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-16 Thread via GitHub


ephraimbuddy commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1566866008


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)
+trigger_kwargs_encryption_version = _REVISION_HEADS_MAP["2.9.0"]

Review Comment:
   ```suggestion
   trigger_kwargs_encryption_revision = _REVISION_HEADS_MAP["2.9.0"]
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-16 Thread via GitHub


ephraimbuddy commented on code in PR #38876:
URL: https://github.com/apache/airflow/pull/38876#discussion_r1566863812


##
airflow/utils/db.py:
##
@@ -1666,11 +1667,14 @@ def upgradedb(
 _reserialize_dags(session=session)
 add_default_pool_if_not_exists(session=session)
 synchronize_log_template(session=session)
-if _revision_greater(
-config,
-_REVISION_HEADS_MAP["2.9.0"],
-_get_current_revision(session=session),
+current_version = _get_current_revision(session=session)

Review Comment:
   `current_version == _from_revision`, No?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-15 Thread via GitHub


hussein-awala commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2057810427

   > Hi @hussein-awala , can you add more information to the commit message, 
I'm finding it difficult to understand what you worked on.
   
   I updated the PR description, could you take a look?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org



Re: [PR] Fix trigger_kwargs encryption/decryption on db migration [airflow]

2024-04-14 Thread via GitHub


ephraimbuddy commented on PR #38876:
URL: https://github.com/apache/airflow/pull/38876#issuecomment-2054195117

   Hi @hussein-awala , can you add more information to the commit message, I'm 
finding it difficult to understand what you worked on.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org