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

Add a setup script #693

Merged
merged 21 commits into from
May 2, 2024
Merged

Conversation

LDannijs
Copy link
Contributor

@LDannijs LDannijs commented Nov 9, 2023

Summary

At the moment, users are required to create a bunch of files containing boilerplate code that they then need to edit whenever they want to add a vendor or new devices. This PR add 2 scripts that create all of the files by just entering a few questions. I'd like to receive feedback on what other changes could be made to improve it.

This is an in between feature, idea is that eventually a type of form will be used for the repo, but for now this should at least be helpful.

(recreated this PR cause i had some organizing of my fork)

Changes

  • add 2 scripts. one for vendor + devices and one for just devices.

Preview of what the scripts look like.
image

image

@Jaime-Trinidad
Copy link
Contributor

@johanstokking can you take a look to @LDannijs work here

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Nice.

To avoid duplication can you call the setup devices script from the setup script? You can pass the vendor ID as argument so you can skip that prompt.

setup Outdated Show resolved Hide resolved
setup Outdated Show resolved Hide resolved
setup Outdated Show resolved Hide resolved
@LDannijs
Copy link
Contributor Author

Decided to make it even easier by just compacting it all down to 1 script with a selection menu. Preview:

Screenshot 2023-11-27 at 15 05 49

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

Thanks.

Few more things:

  1. Add line breaks before 80 characters (that is the max line length)
  2. Respect NO_COLOR environment variable. If set, don't print colors. Not all terminals are able to show them and so it becomes an unreadable mess. You can keep the assignments but make sure they're empty when NO_COLOR is set

@LDannijs
Copy link
Contributor Author

  1. Add line breaks before 80 characters (that is the max line length)

Would I need to do this for only the echo commands or the entire code?

@johanstokking
Copy link
Member

Ideally both but certainly anything printed.

@LDannijs
Copy link
Contributor Author

I hope the NO_COLOR variable is properly implemented. It doesnt seem to be that standardized so I hope this is good.

Copy link
Member

@johanstokking johanstokking left a comment

Choose a reason for hiding this comment

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

LGTM

@johanstokking
Copy link
Member

@Jaime-Trinidad did you test this locally?

@Jaime-Trinidad
Copy link
Contributor

Yes, there are some fixes to do:

Codec:
Just fill uplinkDecoder with the decoder file name, few device makers fill downlinkEncoder and downlinkDecoder and they won't notice and cause errors.

Profile:
Same can we leave macVersion empty it fills with 1.0.3 some of them may think it is ok and won't change it, so they probably don't autofill the info.

Index:
@LDannijs check because I added from an existing vendor and it deletes the already info:

image

@LDannijs
Copy link
Contributor Author

LDannijs commented Dec 8, 2023

(apologies for messing up the commit history. There were commits from a different branch that i had accidentally been using on here, so i tried to fix it 😓. Need to learn to double check these things)

@LDannijs
Copy link
Contributor Author

LDannijs commented Dec 8, 2023

So few changes made:

  • Left macVersion empty and removed downlinkEncoder and downlinkDecoder
  • Made sure the index file in a company folder isn't overwritten if devices already exist in it.
  • Added a check for the vendorProfileID so it increments properly.
  • Also added a check if there is an available company id in the main index.yaml file.

@johanstokking
Copy link
Member

Please ping @Jaime-Trinidad to review and test.

@LDannijs
Copy link
Contributor Author

LDannijs commented Mar 6, 2024

Did a few things:

  • use bash instead of shell for better compatibility (linux especially)
  • removed vendorProfileID check and creation (because it will be replaced with vendorIDs in Validate features #736, so wait with pushing this PR until that one is done)
  • make sure device name is at least 3 characters long to comply with validation
  • Add profile creation step to let user decide which ones they want to add.
  • check the index.yaml file of a vendor to make sure a device with the same name isnt overwritten when adding a new one
    @Jaime-Trinidad

@LDannijs LDannijs marked this pull request as draft March 25, 2024 13:12
@LDannijs
Copy link
Contributor Author

Turned the command to run the script into a make command to match the other commands.

And with #736 now being merged i can reopen this PR.

@LDannijs LDannijs marked this pull request as ready for review April 12, 2024 11:51
@Jaime-Trinidad Jaime-Trinidad merged commit 14190b7 into TheThingsNetwork:master May 2, 2024
2 checks passed
@LDannijs LDannijs deleted the setup-script branch June 11, 2024 10:56
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.

3 participants