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

Chore: Extension generation improvements #2300

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Aditi-Singh16
Copy link
Contributor

@Aditi-Singh16 Aditi-Singh16 commented Mar 10, 2023

Fixes for #1957

@netlify
Copy link

netlify bot commented Mar 10, 2023

Deploy Preview for specter-desktop-docs canceled.

Name Link
🔨 Latest commit cc134b7
🔍 Latest deploy log https://app.netlify.com/sites/specter-desktop-docs/deploys/645607da11acf80008bf4ea6

@Aditi-Singh16 Aditi-Singh16 marked this pull request as draft March 10, 2023 18:28
@Aditi-Singh16 Aditi-Singh16 marked this pull request as ready for review March 16, 2023 17:42
@Aditi-Singh16 Aditi-Singh16 changed the title chore: Extension generation improvements Chore: Extension generation improvements Mar 16, 2023
@Aditi-Singh16
Copy link
Contributor Author

@k9ert , could you please take a look?

.cirrus.yml Outdated
@@ -109,7 +109,7 @@ extension_smoketest_task:
- mkdir tmp && cd tmp
- mkdir testextension && cd testextension
- pwd
- python3 -m cryptoadvance.specter ext gen --ext-id cicdtest --org cryptoadvance --no-isolated-client --devicename cicddevice
- python3 -m cryptoadvance.specter ext gen --ext-id cicdtest --org cryptoadvance --no-isolated-client --no-encrypted-userdata --devicename cicddevice --version v2.0.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the version should be omitted here.

Suggested change
- python3 -m cryptoadvance.specter ext gen --ext-id cicdtest --org cryptoadvance --no-isolated-client --no-encrypted-userdata --devicename cicddevice --version v2.0.0
- python3 -m cryptoadvance.specter ext gen --ext-id cicdtest --org cryptoadvance --no-isolated-client --no-encrypted-userdata --devicename cicddevice

We want to know whether the current branch this is currently being tested, works on the master of specterext-dummy. That might be not optimal in some cases but the best we can get for most of the cases where we have chages in a PR which (hopefully) still match the state in master of specterext-dummy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the smoke test fails if I remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I read the issue again and I think, we need a version since templates are becoming more dependent on the version of Specter controlling the generation process and if no version is specified, it should fall back to master.

If the user chooses to add a specific version, it will render extension accordingly, else the tag(branch) will be master and version will be the latest one.

@@ -59,6 +59,37 @@ def loop(self, dt=3600):
)
time.sleep(dt)

def _check_if_version_is_available(self, custom_version):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This check is checking on pypi. But the relevant check is the tag/branch in the specter-ext repo:
https://github.com/cryptoadvance/specterext-dummy/tags

This is what needs to be used when someone is referring to a tag. So can you change that to check whether the tag/branch is existing? You can also put that check as a class-method in the GithubUrlLoader, It doesn't need to be in the util package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will do that

@@ -46,13 +48,20 @@ def __init__(
self.base_path = base_path
self.org = ext_org
self.id = ext_id
self.encrypted_userdata = encrypted_userdata
self.tag = tag
Copy link
Collaborator

Choose a reason for hiding this comment

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

You introduce here self.tag which is a better name than self.branch. Can you remove the self.branch then and adjust the usage? E.g. the GithubUrlLoader currently has a parameter called "branch" but it should be called tag (and initialized with your self.tag.

@k9ert
Copy link
Collaborator

k9ert commented Mar 23, 2023

In #1957 i also talk about necessary changes in the template-directory:

The service and the controller would needs some adjustments. They are rendered here: https://github.com/cryptoadvance/specter-desktop/blob/master/src/cryptoadvance/specter/cli/cli_ext.py#L150-L160 however, the code for the templates is living here:

https://github.com/cryptoadvance/specterext-dummy/blob/master/src/dummyorg/specterext/dummy/service.py#L12
https://github.com/cryptoadvance/specterext-dummy/blob/master/src/dummyorg/specterext/dummy/controller.py#L53

That would need to be done as well. Please create PR there.

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