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

Make platform_id unnecessary when skipping trusted upload signing #4065

Merged

Conversation

johnathan79717
Copy link
Contributor

The original check works for the quick upload form because it passes the platform param. The full upload form only passes an optional platform param for Android so we shouldn't require it to skip trusted upload signing. The job.lower should be enough in that case.

@johnathan79717
Copy link
Contributor Author

I was finally able to test the change locally. I had to hardcode the following:

  • Set isChromium to True in upload_testcase.py.
  • Comment out the missing target name check in upload_testcase.py.
  • Set is_remotely_executing_utasks() to True.

I also had to manually add jobs at /jobs. I added a linux job and a windows job. Before this change, both jobs require signing in the full upload. After this change, only the windows job requires signing.

@jonathanmetzman
Copy link
Collaborator

/gcbrun

Copy link
Collaborator

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonathanmetzman
Copy link
Collaborator

/gcbrun

@johnathan79717
Copy link
Contributor Author

johnathan79717 commented Jul 18, 2024

I fixed the lint error on the lines I touched. I tried to fix the errors on lines I didn't touch but couldn't find a way to fix two contradicting errors at the same time.

Currently, this lint rule fails:

Running: pylint --score=no --jobs=0 --ignore=protos,tests,grammars clusterfuzz src/appengine/handlers/upload_testcase.py
[1991](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1992)| ************* Module src.appengine.handlers.upload_testcase
[1992](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1993)| src/appengine/handlers/upload_testcase.py:39:0: C0411: third party import "from handlers import base_handler" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1993](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1994)| src/appengine/handlers/upload_testcase.py:40:0: C0411: third party import "from libs import access" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1994](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1995)| src/appengine/handlers/upload_testcase.py:41:0: C0411: third party import "from libs import form" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1995](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1996)| src/appengine/handlers/upload_testcase.py:42:0: C0411: third party import "from libs import gcs" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1996](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1997)| src/appengine/handlers/upload_testcase.py:43:0: C0411: third party import "from libs import handler" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1997](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1998)| src/appengine/handlers/upload_testcase.py:44:0: C0411: third party import "from libs import helpers" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)
[1998](https://github.com/google/clusterfuzz/actions/runs/9979858461/job/27582719607?pr=4065#step:5:1999)| src/appengine/handlers/upload_testcase.py:45:0: C0411: third party import "from libs.query import datastore_query" should be placed before "from clusterfuzz._internal import fuzzing" (wrong-import-order)

However, if I move those imports above to make this rule happy, another rule fails:

⋊> ~/clusterfuzz on fix-trusted-agreement-signing ⨯ isort --dont-order-by-type --force-single-line-imports --force-sort-within-sections --line-length=80 -p clusterfuzz 
-p handlers -p libs  --df src/appengine/handlers/upload_testcase.py
--- /usr/local/google/home/phao/clusterfuzz/src/appengine/handlers/upload_testcase.py:before    2024-07-18 08:55:44.732096
+++ /usr/local/google/home/phao/clusterfuzz/src/appengine/handlers/upload_testcase.py:after     2024-07-18 08:56:00.073250
@@ -22,13 +22,6 @@
 from flask import request
 from google.cloud import ndb
 
-from handlers import base_handler
-from libs import access
-from libs import form
-from libs import gcs
-from libs import handler
-from libs import helpers
-from libs.query import datastore_query
 from clusterfuzz._internal import fuzzing
 from clusterfuzz._internal.base import external_users
 from clusterfuzz._internal.base import memoize
@@ -43,6 +36,13 @@
 from clusterfuzz._internal.issue_management import issue_tracker_utils
 from clusterfuzz._internal.system import archive
 from clusterfuzz._internal.system import environment
+from handlers import base_handler
+from libs import access
+from libs import form
+from libs import gcs
+from libs import handler
+from libs import helpers
+from libs.query import datastore_query
 
 MAX_RETRIES = 50
 RUN_FILE_PATTERNS = ['run', 'fuzz-', 'index.', 'crash.']

@jonathanmetzman What can I do? Is it okay to merge it as is?

@jonathanmetzman
Copy link
Collaborator

Put # pylint: disable=wrong-import-order before the imports please.

…uirement

The original check works for the quick upload form because it passes the
platform param.  The full upload form only passes an optional platform
param for Android so we shouldn't require it to skip trusted upload
signing.  The job.lower should be enough in that case.
@johnathan79717
Copy link
Contributor Author

Put # pylint: disable=wrong-import-order before the imports please.

Done.

@jonathanmetzman
Copy link
Collaborator

/gcbrun

@jonathanmetzman
Copy link
Collaborator

If this passes can you remind me to deploy it on Monday. I'm traveling/stranded by crowdstrike today

@jonathanmetzman jonathanmetzman merged commit fba2390 into google:master Jul 22, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants