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

DiscoverResponse should represent exist resources. #530

Closed
rkimsb2 opened this issue Jul 6, 2018 · 13 comments · Fixed by #532
Closed

DiscoverResponse should represent exist resources. #530

rkimsb2 opened this issue Jul 6, 2018 · 13 comments · Fixed by #532

Comments

@rkimsb2
Copy link
Contributor

rkimsb2 commented Jul 6, 2018

But it just represents the model infos.

@rkimsb2 rkimsb2 changed the title DiscoverResponse on object instance should represent all resource path it has own. DiscoverResponse should represent exist resources. Jul 6, 2018
@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 6, 2018

I think this PR solve this issue. but there's another issue.
Object Instance or Resource cannot represent it's own attributes.
This issue has been mentioned on #369 issue.

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 6, 2018

OMG. This PR has another problem. The path of Executable Resource cannot be represented.
In the case of optional executable resource, could we know that resource is implemented?

@sbernard31
Copy link
Contributor

@rkimsb2, I didn't find time to look at all your contribution. I hope I should be able to do that tomorrow.

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 11, 2018

Ok I looked at the #531 PR and you right there is issue about Executable and Writable resource. I also think this is not a so good idea to read the whole instance to know if the optional resource is implemented or not.

I think the strategy choosen by @danielhqv with a getAvailableResources() is the good one.

Let me try to propose a PR inspired by your 2 PRs.

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 11, 2018

@rkimsb2, I pushed a new PR to fix this issue : #532.

Could you have a look and tell us if it's fit your needs ?

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 12, 2018

Gladly. I will test this PR tomorrow and leave comments. :)

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 13, 2018

OK. This PR can solve the problem of discovering executable resource.

And.. there is still left some problem.
The attributes are not displayed in discover response.
But this problem is already mentioned as not supported on #369 issue.

I think this issue can be closed with this PR.

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 13, 2018

And I ask carefully, do you have some plans for supporting attributes on client side?
or could we open the issue for that?

IMHO, if we make some layer something like ResourceEnabler, we can support not just attributes, also support the working of notification attributes.

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 13, 2018

The attributes are not displayed in discover response.

Yep, still not supported. #484 (comment) is a sum up of the progress on this feature.

do you have some plans for supporting attributes on client side?

My next priority is more about supporting X509 and RPK at client side, but if I find time we can try to move forward on this feature.

could we open the issue for that?

Yes, we can ! ;)

IMHO, if we make some layer something like ResourceEnabler, we can support not just attributes, also support the working of notification attributes.

I'm not sure to see the benefits of adding a ResourceEnabler ?
If you are curious about this feature, you can have a look to #484 but there is a lot to read and filter ;)

About #532 and #533 would you like to review it ? or could I merge it in master ?

I think this issue can be closed with this PR.

I close it as soon as #532 is integrated in master.

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 13, 2018

In my opinion, PR #532 can close this issue. 👍

@rkimsb2
Copy link
Contributor Author

rkimsb2 commented Jul 13, 2018

And about PR #533, I have no idea the PR related with this issue.
but for polishing write attributes, i think it can be merged to master. :)

@sbernard31
Copy link
Contributor

Thx @rkimsb2 :)

@sbernard31
Copy link
Contributor

I create a specific issue for support of write attributes : #534

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 a pull request may close this issue.

2 participants