Set default sigkey if user doesn't insert one #536

xiangge · 2018-02-11T11:29:03Z

Set default sigkey if user doesn't insert one

lubomir

I'm not sure about changing the value on update: can users still create a release without a sigkey if they want to?

lubomir · 2018-02-12T07:28:10Z

pdc/settings_local.py.dist

@@ -84,6 +84,9 @@ EMAIL_SUBJECT_PREFIX = '[PDC]'
 # Omit sending e-mails about big data change when author is in this whitelist.
 #BIG_CHANGE_AUTHORS_WHITELIST = ('some_user',)

+# Set the key_id of sigkey "redhatrelease2" as default


It's not a key_id, but name of the key. Also maybe a dummy value would be better as an example (e.g. Gold Key or something)?

Here will get the Sigkey object by the default key name, this is also set in et side.
And the name is also unique and not null. And we should keep the sigkey table with the default key_id/name before create a release.

lubomir · 2018-02-12T07:28:32Z

pdc/settings_test.py

@@ -48,3 +48,6 @@
 }

 COMPONENT_BRANCH_NAME_BLACKLIST_REGEX = r'^epel\d+$'
+
+# Set the key_id of sigkey "redhatrelease2" as default


The comment should say name of key, not key_id.

Yes, will change it.

lubomir · 2018-02-12T07:40:21Z

pdc/apps/release/serializers.py

+        except Exception:
+            # If there is no the default SigKey object, just keep it as null.
+            pass
+        return validated_data


If I understand correctly:

if the config option is missing, an error will be reported to users

if the config specifies a key that does not exist, nothing will happen

To me it this seems unintuitive: I would expect to be told when I make a typo in the configuration and the key does not exist. Also when the config is not given, it might work similarly to how it did before.

Yep, correct, should change it. Here we want the default sigkey must in the database. So I think we should raise error on both Exception.

xiangge · 2018-02-13T07:37:22Z

Commit new changes, please help to review. And this pr is based on #533

@lubomir User can still insert the data without sigkey, so nothing needs change in ct's input data or scripts.
The default sigkey will set as it is config in the settings file, and also the default key must include in the db first, otherwise it will raise error.

hluk · 2018-02-23T11:43:28Z

pdc/settings_local.py.dist

@@ -84,6 +84,9 @@ EMAIL_SUBJECT_PREFIX = '[PDC]'
 # Omit sending e-mails about big data change when author is in this whitelist.
 #BIG_CHANGE_AUTHORS_WHITELIST = ('some_user',)

+# Set the name of sigkey "redhatrelease2" as default


Explain the option in comment instead.

Added it in the comment

hluk · 2018-02-23T11:46:01Z

pdc/settings_local.py.dist

@@ -84,6 +84,9 @@ EMAIL_SUBJECT_PREFIX = '[PDC]'
 # Omit sending e-mails about big data change when author is in this whitelist.
 #BIG_CHANGE_AUTHORS_WHITELIST = ('some_user',)

+# Set the name of sigkey "redhatrelease2" as default
+RELEASE_DEFAULT_SIGKEY = "redhatrelease2"


Choose some generic example name, this value looks like it's used somewhere internally.

If you mean "redhatrelease2", this is the name which is set in ET, can't change.
And as another pr is canceled, here it should be the id not a name. Will change it.
In my understanding, this sigkey is used for ET, so we should keep it same with ET.

hluk · 2018-02-23T15:19:40Z

pdc/settings_test.py

@@ -48,3 +48,6 @@
 }

 COMPONENT_BRANCH_NAME_BLACKLIST_REGEX = r'^epel\d+$'
+
+# Set the name of sigkey as default


Remove the comment.

Have already removed that.

hluk · 2018-02-23T15:22:33Z

pdc/apps/release/tests.py

@@ -1196,7 +1207,7 @@ def test_update_can_explicitly_erase_optional_field(self):
        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertIsNone(response.data.get('dist_git'))
        self.assertIsNone(models.Release.objects.get(release_id='release-1.0').dist_git_branch)
-        self.assertNumChanges([1])
+        self.assertNumChanges([2])


What's the additional change about?

I was also confused here. After I print the changeset, I really saw two changes.
Here it is:
[{'old_value': u'{"release_id": "release-1.0", "product_version": null, "integrated_with": null, "sigkey": null, "active": true, "allow_buildroot_push": false, "short": "release", "name": "Test Release", "allowed_debuginfo_services": [], "version": "1.0", "allowed_push_targets": [], "base_product": null, "release_type": "ga"}', 'target_id': 1, 'new_value': u'{"release_id": "release-1.0", "product_version": null, "integrated_with": null, "sigkey": "ABCDEF", "active": true, "allow_buildroot_push": false, "short": "release", "name": "Test Release", "allowed_debuginfo_services": [], "version": "1.0", "allowed_push_targets": [], "base_product": null, "release_type": "ga"}', 'target_class': u'release', u'changeset_id': 1, u'id': 1},
{'old_value': u'{"dist_git_branch": "release_branch", "release_id": "release-1.0"}', 'target_id': 1, 'new_value': u'null', 'target_class': u'releasedistgitmapping', u'changeset_id': 1, u'id': 2}]

I think the original one is just dist_git_branch change, but after set the default sigkey, here is one release change.

This is correct: dist git mapping is stored in a separate model, and now also the release itself is changing.

hluk · 2018-02-26T08:18:25Z

pdc/apps/release/tests.py

+    def test_create_with_default_sigkey(self):
+        args = {"name": "TestSigkey", "short": "supp", "version": "1.1",
+                "release_type": "ga", "base_product": "product-1"}
+        # settings.configure(RELEASE_DEFAULT_SIGKEY= 'ABCDEF')


hluk · 2018-02-26T09:45:49Z

pdc/settings_local.py.dist

@@ -24,6 +24,8 @@
 # checks, uncomment this line, set DISABLE_RESOURCE_PERMISSION_CHECK with True.
 #     DISABLE_RESOURCE_PERMISSION_CHECK = True
 #
+# Example 3: set the id of sigkey which has name "redhatrelease2" as default


From this description I still wouldn't know what the option does.

Add more comment

hluk · 2018-02-26T10:04:16Z

pdc/apps/release/serializers.py

+                validated_data["sigkey"] = SigKey.objects.get(key_id=settings.RELEASE_DEFAULT_SIGKEY)
+        except AttributeError:
+            # If there is no Settings.RELEASE_DEFAULT_SIGKEY, raise it in server log
+            raise AttributeError('Settings.RELEASE_DEFAULT_SIGKEY does not exist, please set one with the Sigkey key_id.')


In a comment you say:

User can still insert the data without sigkey, so nothing needs change in ct's input data or scripts.

but it doesn't seem to be possible here.

Can you also add tests for that?

Note: You can override settings in Django like this.

This try/catch just checks if the default key includes in db.
And it means if people not insert one sigkey, will set a default one, so the users' scripts don't need change, they can still add the release without a sigkey input.

But it requires RELEASE_DEFAULT_SIGKEY option to be set and refer to an existing sigkey. Right? This will require manual update of PDC server's configuration and data after upgrading to a new version.

Also, I don't understand where the default values ("redhatrelease2", "fd431d51") come from. Any specific data shouldn't be pushed to this repository.

Yes, the default sigkey should be included into the db first.
And the default sigkey shouldn't change once it set even doing the upgrade. This ("redhatrelease2", "fd431d51") is set as default in ET's settings. And also this is already includes in the ET's database. So we should keep the data same as ET's.

What about Fedora? Those values should be in ansible repo; pdc/settings_local.py.dist is just template for the configuration file, ideally, it should also serve as documentation/reference for the options.

aha, good question, I forgot one thing, if we close the pr about the sigkey name's changes, we can do like SigKey.get_cached_id does, create one if it doesn't have.

hluk · 2018-02-28T10:11:08Z

There are still issues in this PR:

Specific key IDs should not be pushed in the repo. This should go to respective ansible/infrastructure repos.
The new option RELEASE_DEFAULT_SIGKEY should be optional -- don't force people to use a sigkey when creating releases -- so it doesn't break existing API behavior.

xiangge · 2018-03-01T03:24:19Z

@hluk
Good idea, here in et the pdc's release is same as legacy product-version, so the release must have a sigkey (a product-version with a sigkey in et). We can set it in ansible playbooks, and RELEASE_DEFAULT_SIGKEY can be optional and not influence the fedora side then.

hluk · 2018-03-01T06:48:43Z

pdc/apps/release/serializers.py

+                if "sigkey" in validated_data and not validated_data["sigkey"]:
+                    validated_data["sigkey"], _ = SigKey.objects.get_or_create(key_id=settings.RELEASE_DEFAULT_SIGKEY)
+        except Exception, e:
+            raise Exception('Error(%s) when get Key_id(%s).' % (e, settings.RELEASE_DEFAULT_SIGKEY))


Is the except block needed? What can happen here?

Actually, it's no need, but I think it's also ok to track some unexpected errors.

You can say that about a lot of code. It actually raises different type of exception here which can change how Django reacts.

So I would rather if you remove it.

Make sense to me.

hluk · 2018-03-01T06:57:01Z

pdc/settings_local.py.dist

+# Example 3: set the sigkey's key_id fd431d51 as default, this default key_id is
+# used when insert a release into database but without a sigkey, it will be set
+# into the release. This default sigkey's name is "redhatrelease2".
+#     RELEASE_DEFAULT_SIGKEY = "fd431d51"


Remove the key IDs.

I don't like the examples here are used to describe options, it's bit difficult to read. Can you move out and use similar format as for options below (DEBUG_TOOLBAR, BIG_CHANGE_AUTHORS_WHITELIST)?

It doesn't describe what happens if the key doesn't exist.

I am not sure if I can get your point here, I have committed patch for the ansible codes, to set the RELEASE_DEFAULT_SIGKEY there.
And this comment is for other people like fedora guys who want to build a pdc server, they can choose if to set this RELEASE_DEFAULT_SIGKEY.

I understand, but why mention these particular key IDs?

Aha, this is et's default key, as I have set it in ansible codes, not sure if we can remove this to set a fake one, so keep it here for the review :)

I think it's OK to use the same value as for tests (provided the option is commented-out).

Have changed the name to a non-existent one "Sigkey's key_id"

hluk · 2018-03-01T09:04:47Z

@lubomir Can you review?

xiangge · 2018-03-01T09:13:12Z

@lubomir can you help to review it?

lubomir · 2018-03-01T09:53:35Z

pdc/apps/release/tests.py

@@ -1196,7 +1207,7 @@ def test_update_can_explicitly_erase_optional_field(self):
        self.assertEqual(response.status_code, status.HTTP_200_OK)
        self.assertIsNone(response.data.get('dist_git'))
        self.assertIsNone(models.Release.objects.get(release_id='release-1.0').dist_git_branch)
-        self.assertNumChanges([1])
+        self.assertNumChanges([2])


This is correct: dist git mapping is stored in a separate model, and now also the release itself is changing.

lubomir · 2018-03-01T09:55:08Z

pdc/settings_local.py.dist

+# Example 3: when you insert a release into database without a sigkey data, the sigkey
+# will be null by default, you can also choose if set the key_id as default, this default
+# key_id will be added into the data when insert a releaseinto database without a sigkey.
+#     RELEASE_DEFAULT_SIGKEY = "Sigkey's key_id"


Minor typo here releaseinto → release into.

Thanks for the review :)

Set default sigkey when insert release with null sigkey JIRA: PDC-2567

xiangge requested review from hluk and lubomir February 11, 2018 11:29

xiangge force-pushed the sigkey-not-null-in-release branch 2 times, most recently from 839f11a to 16470be Compare February 12, 2018 00:24

lubomir reviewed Feb 12, 2018

View reviewed changes

xiangge force-pushed the sigkey-not-null-in-release branch from 7eef966 to 800e237 Compare February 13, 2018 07:32

xiangge force-pushed the sigkey-not-null-in-release branch from 800e237 to 60f1abd Compare February 13, 2018 07:39

xiangge force-pushed the sigkey-not-null-in-release branch from 60f1abd to b046e66 Compare February 23, 2018 02:11

hluk requested changes Feb 23, 2018

View reviewed changes

hluk requested changes Feb 26, 2018

View reviewed changes

xiangge force-pushed the sigkey-not-null-in-release branch 2 times, most recently from edbcc4b to 339a455 Compare February 27, 2018 01:58

xiangge force-pushed the sigkey-not-null-in-release branch from 339a455 to 3147c0b Compare March 1, 2018 06:22

hluk reviewed Mar 1, 2018

View reviewed changes

xiangge force-pushed the sigkey-not-null-in-release branch from 3147c0b to 2f9ff2d Compare March 1, 2018 08:00

hluk approved these changes Mar 1, 2018

View reviewed changes

lubomir approved these changes Mar 1, 2018

View reviewed changes

xiangge force-pushed the sigkey-not-null-in-release branch 2 times, most recently from 98e147d to 6717811 Compare March 1, 2018 10:52

Set default sigkey when insert release with null sigkey

86769ba

Set default sigkey when insert release with null sigkey JIRA: PDC-2567

xiangge force-pushed the sigkey-not-null-in-release branch from 6717811 to 86769ba Compare March 2, 2018 00:37

hluk merged commit 44d433b into product-definition-center:master Mar 2, 2018

Set default sigkey if user doesn't insert one #536

Set default sigkey if user doesn't insert one #536

Conversation

xiangge commented Feb 11, 2018

lubomir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangge Feb 12, 2018 • edited

Choose a reason for hiding this comment

xiangge commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangge Feb 27, 2018 • edited

Choose a reason for hiding this comment

hluk commented Feb 28, 2018

xiangge commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hluk commented Mar 1, 2018

xiangge commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangge Feb 12, 2018 •

edited

xiangge Feb 27, 2018 •

edited