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.

Sign up for GitHub

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

Jump to bottom

Initial commit for Cors System Organisation #801

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

Initial commit for Cors System Organisation #801

wants to merge 20 commits into from

Conversation

patientenvy
Copy link

Summary

...

Changes

  • Added cortu-genesis 2 device

Checklist for Reviewers

  • Title and description should be descriptive (Not just a serial number for example).
  • profileIDs should not be vendorID and should be a unique value for every profile.
  • All devices should be listed in the vendor's index.yaml file.
  • Firmware versions can not be changed.
  • At least 1 image per device and should be transparent.

Notes for Reviewers

...

Release Notes

  • ...

@CLAassistant
Copy link

CLAassistant commented Jul 15, 2024 •
edited
Loading

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ cors-oreoluwa
✅ patientenvy
❌ Aceallio
You have signed the CLA already but the status is still pending? Let us recheck it.

LDannijs
LDannijs reviewed Jul 18, 2024
View reviewed changes
vendor/corssystem/cortu-gen-3.yaml Outdated
@@ -0,0 +1,192 @@
name: Wind Sensor # Device name can not contain the vendor name
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the majority of files you added contain the example wind sensor files. Could you fix this?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, I've fixed it, the file was still being drafted

LDannijs
LDannijs suggested changes Jul 18, 2024
View reviewed changes
Copy link
Contributor

@LDannijs LDannijs left a comment

Choose a reason for hiding this comment

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

The validation is giving formatting errors too, please run make validate fmt

vendor/corssystem/cortugenesis.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This file contains the decoder for the example wind sensor as well. Do you have your own decoder or?

Copy link
Author

Choose a reason for hiding this comment

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

I've made all the changes

vendor/corssystem/index.yaml Outdated Show resolved Hide resolved
vendor/corssystem/cortugenesis-codec.yaml Outdated Show resolved Hide resolved
@patientenvy patientenvy marked this pull request as draft July 20, 2024 07:08
@patientenvy patientenvy marked this pull request as ready for review August 2, 2024 10:01
@LDannijs
Copy link
Contributor

@patientenvy Files look good now, could you please still run make validate fmt?
(apologies for being so late, i was on vacation :)

@CorsInterns
Copy link

CorsInterns commented Aug 26, 2024 •
edited
Loading

I have run the make validate fmt and it worked but it seems like there is a problem with github check format. It runs perfectly locally on my computer
problem
ptest

@LDannijs
Copy link
Contributor

Could you please run make validate fmt specifically? Our validation is specifically made for our repo.

LDannijs
LDannijs reviewed Aug 27, 2024
View reviewed changes
package.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems youve changed some things in the package files? Could you revert this?

@CorsInterns
Copy link

I have reverted the package files

LDannijs
LDannijs reviewed Aug 28, 2024
View reviewed changes
package.json Outdated
@@ -22,7 +22,7 @@
"homepage": "https://github.com/TheThingsNetwork/lorawan-devices#readme",
"devDependencies": {
"ajv": "^6.12.6",
"ajv-cli": "^5.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

There's still some changes here. Please revert these.

LDannijs
LDannijs reviewed Aug 28, 2024
View reviewed changes
package-lock.json Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This file also shows changes being made. Please revert this too.

@LDannijs
Copy link
Contributor

CLA assistant check Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.1 out of 3 committers have signed the CLA.✅ patientenvy❌ cors-oreoluwa❌ AceallioYou have signed the CLA already but the status is still pending? Let us recheck it.

Also, please sign our CLA..

@LDannijs
Copy link
Contributor

LDannijs commented Sep 9, 2024

@Aceallio could you sign the CLA still?

@LDannijs
Copy link
Contributor

@Aceallio Any update here? Could you please sign the CLA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LDannijs LDannijs LDannijs requested changes

@CorsInterns CorsInterns CorsInterns approved these changes

@Jaime-Trinidad Jaime-Trinidad Jaime-Trinidad approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@patientenvy @CLAassistant @LDannijs @CorsInterns @Jaime-Trinidad @Aceallio @cors-oreoluwa