a fix for duplicate alias problem #3010

godplayer56 · 2023-10-12T21:19:22Z

This pull request tries to fix the duplicate-alias issue #2942

godplayer56 · 2023-10-13T10:56:25Z

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.

I had used the same function which has been used for rules. How should I change the function for the test to pass?

bhearsum · 2023-10-13T19:41:31Z

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.

I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

godplayer56 · 2023-10-13T21:04:02Z

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.
I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

@bhearsum How should I approach fixing the python formatting issue? How should I check if the files have formatting issues?

I am thinking of adding a test similar to the test for checking alias in rules class

bhearsum · 2023-10-16T14:21:31Z

@bhearsum @gabrielBusta I am seeing that one of the tests failed for the PR 😵‍💫.
I had used the same function which has been used for rules. How should I change the function for the test to pass?

I can see that the failure is a bit confusing -- this is actually a failure in the Python file formatting - not in any test.

[task 2023-10-12T21:23:33.129Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/black --diff --check scripts src taskcluster tests uwsgi (exited with code 1)

In addition to fixing that, it would be good to add tests for this in https://github.com/mozilla-releng/balrog/blob/main/tests/admin/views/test_rules.py (we already have a number of tests for these endpoints that should be a good model for new ones).

@bhearsum How should I approach fixing the python formatting issue? How should I check if the files have formatting issues?

I am thinking of adding a test similar to the test for checking alias in rules class

You can use black to format the Python files correctly.

godplayer56 · 2023-10-16T14:49:26Z

I have formatted the code but some tests still fail

bhearsum · 2023-10-16T15:11:21Z

I have formatted the code but some tests still fail

Please have a look through the linked log for any errors that are still showing up.

godplayer56 · 2023-10-16T21:17:19Z

I have formatted the code but some tests still fail

Please have a look through the linked log for any errors that are still showing up.

I checked for errors but only warnings are being shown. I have formatted all the files using black, so it can be some other issue which I am not able to figure out from the logs 😢

gbrownmozilla · 2023-10-16T22:23:52Z

Sorry, this shouldn't be so hard!

The log shown by github is incomplete; you can see the full test log if you "View logs in Taskcluster". In the full log, I see

[task 2023-10-16T10:43:40.522Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/check-manifest -v . (exited with code 1)

...which doesn't tell you much! I wonder if that's related to the new file you are adding? I would suggest not including settings.json in the PR.

godplayer56 · 2023-10-17T13:22:06Z

Sorry, this shouldn't be so hard!

The log shown by github is incomplete; you can see the full test log if you "View logs in Taskcluster". In the full log, I see
[task 2023-10-16T10:43:40.522Z] ERROR: InvocationError for command /builds/worker/checkouts/vcs/.tox/check/bin/check-manifest -v . (exited with code 1)
...which doesn't tell you much! I wonder if that's related to the new file you are adding? I would suggest not including settings.json in the PR.

Oh I checked the taskcluster logs but I wasn't able to see them 😞 , I'll be careful from now on! Here is what I see in the taskcluster logs even after logging in-

For the settings.json file, I will remove it from the PR.

godplayer56 · 2023-10-17T14:48:44Z

Hii @bhearsum @gbrownmozilla , the checks have passed, can you please review the PR?

Should I do some changes to it?

bhearsum · 2023-10-18T00:37:07Z

tests/admin/views/test_rules.py

@@ -2834,3 +2834,14 @@ def testCannotRevokeOtherUsersSignoffAsNormalUser(self):
        ret = self._delete("/scheduled_changes/rules/3/signoffs", username="julie", qs={"username": "mary"})
        self.assertEqual(ret.status_code, 403, ret.get_data())
        self.assertEqual(ret.mimetype, "application/json")
+
+    def testScheduledChangeWithDuplicateAlias(self):


This test is not quite right, unfortunately. It is testing the /rules view - which is already covered by

balrog/tests/admin/views/test_rules.py

Line 540 in 6493a9e

ret = self._post(

.

You should be adding at least two tests:

One that validates a POST to /scheduled_changes/rules, and one that validates a POST to /scheduled_changes/rules/:sc_id. The first could be modeled after

balrog/tests/admin/views/test_rules.py

Line 2161 in 6493a9e

def testAddScheduledChangeNewRuleWithoutUpdateType(self):

, the second could be modeled after

balrog/tests/admin/views/test_rules.py

Line 2251 in 6493a9e

@mock.patch("time.time", mock.MagicMock(return_value=300))

In both cases the request should a result in a 400 if any of the following are true:

The alias exists in any row in the rules table

The alias exists in any row in the rule_scheduled_changes table that meets both the following conditions:

complete is False

change_type is insert or update

(This may require additional changes to rules.py as well - please let me know if you have any questions.)

I'll start working on updating the test 👍

@bhearsum I have a doubt regarding this -
How do I select the required data in the rule_scheduled_change table? for eg-
for the rules table we have the getAlias function defined to get the rule with the given alias but for rule_scheduled_changes I am not able to figure out how should I select the data with a given alias.

and also does the column scheduled_by denote the alias for the rule_scheduled_changes table?

@bhearsum I have a doubt regarding this - How do I select the required data in the rule_scheduled_change table? for eg- for the rules table we have the getAlias function defined to get the rule with the given alias but for rule_scheduled_changes I am not able to figure out how should I select the data with a given alias.

The scheduled_changes table is accessible through dbo.rules.scheduled_changes (You can see an example query in the tests

balrog/tests/admin/views/test_rules.py

Line 2012 in 6d46207

r = dbo.rules.scheduled_changes.t.select().where(dbo.rules.scheduled_changes.sc_id == 8).execute().fetchall()

- and there's many more example of selects in other tests and parts of the codebase).

and also does the column scheduled_by denote the alias for the rule_scheduled_changes table?

No -- scheduled_by is the person who scheduled the change :). The alias is in the base_alias column in the scheduled changes table (apologies -- I forgot about that detail when I posted the earlier comment).

Ok got it! 😄

godplayer56 · 2023-10-21T15:53:54Z

@bhearsum @gbrownmozilla I have added the tests for the code. Can you please have a look and tell me if it needs some changes?

bhearsum · 2023-10-23T14:25:36Z

@bhearsum @gbrownmozilla I have added the tests for the code. Can you please have a look and tell me if it needs some changes?

It looks like you're making progress. The tests that you added looks fine to me, but you need to cover some additional cases still:

A POST to /scheduled_changes/rules with update_type set to update with a duplicate alias
A POST to /scheduled_changes/rules/:sc_id with update_type set to insert with a duplicate alias

godplayer56 · 2023-10-23T18:48:47Z

Ok got it, so I'd be adding two more tests for it then 👍

godplayer56 · 2023-10-23T19:43:59Z

@bhearsum are the changes fine?

bhearsum · 2023-10-23T20:31:39Z

src/auslib/web/admin/views/rules.py

+                return problem(400, "Bad Request", "Rule with alias exists.")
+
+            if alias is not None:
+                scheduled_rule_with_alias = dbo.rules.scheduled_changes.t.select().where(dbo.rules.scheduled_changes.base_alias == alias).execute().fetchall()


This (and the similar block below) are missing the conditions I called out in #3010 (comment):

complete is False

change_type is insert or update

tests/admin/views/test_rules.py

bhearsum · 2023-10-25T13:36:07Z

@godplayer56 - is this ready for another look? If so, please use the "Re-request review button" in the Reviewers list.

godplayer56 · 2023-10-25T13:38:59Z

@godplayer56 - is this ready for another look? If so, please use the "Re-request review button" in the Reviewers list.

I am trying to resolve the post issues right now. I'll use it as soon as I am done resolving the issue 👍

godplayer56 · 2023-10-25T20:22:20Z

@bhearsum I have added assertions that confirm that the tests are failing for the right reason 👍

bhearsum

It looks like something is not quite complete or right here. In my local database I already have an alias of firefox-release, but I can still scheduled a change to add one on https://localhost:9000/rules/364, which is accepted.

bhearsum · 2023-10-25T23:59:05Z

src/auslib/web/admin/views/rules.py

@@ -260,6 +260,24 @@ def _post(self, transaction, changed_by):
            if what.get("fallbackMapping") is not None and len(fallback_mapping_values) != 1:
                return problem(400, "Bad Request", "Invalid fallbackMapping value. No release name found in DB")

+            alias = what.get("base_alias", None)


I think the problem is that in this part of the view this needs to be referred to as alias (not base_alias). I suggest you try printing out what and making a web request to confirm this.

bhearsum · 2023-10-26T00:00:16Z

tests/admin/views/test_rules.py

+            "mapping": "c",
+            "update_type": "minor",
+            "sc_data_version": 1,
+            "base_alias": "TestDuplicateAlias3",


Also try simulating a request like this on https://localhost:9000/rules/364 by adding an alias and clicking the Save button - take a dlook at what the the Request JSON looks like in the Developer Tools. If it doesn't use base neither should you in these tests.

I got base_alias from the scheduledChangesTable because in some instances it referred to base_alias instead of alias 😅 . Can you provide me with authorization so I can see what a post request looks like?

I have updated all the files to use alias instead of base_alias. Though I can't confirm if the issue is now resolved because I don't have authorization for making updates

You should be fully authorized in your local development environment...

In any case, I can assure you that the admin API doesn't use base in these requests.

bhearsum

This looks good now - thank you!

This reverts commit 994bd6a.

a fix for duplicate alias problem

cf2ce74

godplayer56 requested review from jcristau, gbrownmozilla and gabrielBusta as code owners October 12, 2023 21:19

This commit corrects the formatting for python files

b78811e

godplayer56 added 3 commits October 17, 2023 18:57

removed the settings.json file

9f9b6df

adding test for checking duplicate alias

8f5f13b

fixing formatting errors

9cc67e2

bhearsum requested changes Oct 18, 2023

View reviewed changes

godplayer56 added 8 commits October 19, 2023 00:47

added two tests and updated rules.py

8c5eaec

changes to rules.py

450e1ae

added new condition

e3ed332

added test for /scheduled_changes/rules

898c330

corrected formatting error

59bc8a3

completed the second test

e3fb94c

trying to fix build errors

5ce07b4

formatting errors

2254047

added two more tests with different update types

c870770

bhearsum requested changes Oct 23, 2023

View reviewed changes

adding exception for tests

7fa4c05

fixing backend test errors

399ee9a

godplayer56 added 5 commits October 25, 2023 19:45

added and assertion

9051216

updated tests

2ca2d73

added data_version

167b56a

tests updated

0f6f3cb

updated the error message

c46d589

godplayer56 requested a review from bhearsum October 25, 2023 20:22

bhearsum requested changes Oct 26, 2023

View reviewed changes

base_alias -> alias

aef0681

bhearsum approved these changes Oct 27, 2023

View reviewed changes

bhearsum merged commit 994bd6a into mozilla-releng:main Oct 27, 2023
7 checks passed

This was referenced Oct 27, 2023

Balrog accepts scheduled rule changes with duplicate alias' #2942

Open

Prevent scheduling a new rule with a duplicate alias #3029

Closed

godplayer56 deleted the duplicate-alias branch October 28, 2023 18:03

godplayer56 restored the duplicate-alias branch October 28, 2023 18:03

JohanLorenzo mentioned this pull request Nov 10, 2023

Cannot change an aliased rule anymore: "rule with alias exists" #3048

Closed

JohanLorenzo added a commit that referenced this pull request Nov 13, 2023

Revert "a fix for duplicate alias problem (#3010)"

4d2f475

This reverts commit 994bd6a.

JohanLorenzo mentioned this pull request Nov 13, 2023

Bug 1864286 - Revert "a fix for duplicate alias problem" #3049

Merged

JohanLorenzo added a commit that referenced this pull request Nov 13, 2023

Revert "a fix for duplicate alias problem (#3010)" (#3049)

cbc1977

This reverts commit 994bd6a.

jbuck mentioned this pull request Jun 15, 2024

aus5.mozilla.org 2024 tls certificate renewal #3121

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a fix for duplicate alias problem #3010

a fix for duplicate alias problem #3010

godplayer56 commented Oct 12, 2023

godplayer56 commented Oct 13, 2023

bhearsum commented Oct 13, 2023

godplayer56 commented Oct 13, 2023

bhearsum commented Oct 16, 2023

godplayer56 commented Oct 16, 2023

bhearsum commented Oct 16, 2023

godplayer56 commented Oct 16, 2023

gbrownmozilla commented Oct 16, 2023

godplayer56 commented Oct 17, 2023

godplayer56 commented Oct 17, 2023

bhearsum Oct 18, 2023

godplayer56 Oct 18, 2023

godplayer56 Oct 18, 2023 •

edited

Loading

bhearsum Oct 19, 2023

godplayer56 Oct 19, 2023

godplayer56 commented Oct 21, 2023

bhearsum commented Oct 23, 2023

godplayer56 commented Oct 23, 2023

godplayer56 commented Oct 23, 2023

bhearsum Oct 23, 2023

bhearsum commented Oct 25, 2023

godplayer56 commented Oct 25, 2023

godplayer56 commented Oct 25, 2023

bhearsum left a comment

bhearsum Oct 25, 2023

bhearsum Oct 26, 2023

godplayer56 Oct 26, 2023

godplayer56 Oct 26, 2023 •

edited

Loading

bhearsum Oct 27, 2023

bhearsum left a comment

a fix for duplicate alias problem #3010

a fix for duplicate alias problem #3010

Conversation

godplayer56 commented Oct 12, 2023

godplayer56 commented Oct 13, 2023

bhearsum commented Oct 13, 2023

godplayer56 commented Oct 13, 2023

bhearsum commented Oct 16, 2023

godplayer56 commented Oct 16, 2023

bhearsum commented Oct 16, 2023

godplayer56 commented Oct 16, 2023

gbrownmozilla commented Oct 16, 2023

godplayer56 commented Oct 17, 2023

godplayer56 commented Oct 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godplayer56 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godplayer56 commented Oct 21, 2023

bhearsum commented Oct 23, 2023

godplayer56 commented Oct 23, 2023

godplayer56 commented Oct 23, 2023

Choose a reason for hiding this comment

bhearsum commented Oct 25, 2023

godplayer56 commented Oct 25, 2023

godplayer56 commented Oct 25, 2023

bhearsum 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

godplayer56 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhearsum left a comment

Choose a reason for hiding this comment

godplayer56 Oct 18, 2023 •

edited

Loading

godplayer56 Oct 26, 2023 •

edited

Loading