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

Added method and tests for revoking access #13

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

ishmelev-sp
Copy link
Contributor

I've added a feature that is available already in the uplink-cli, but it isn't presented here. Actually, I need it for uplink-nodejs, I'll propose PR there in a few days.

Also, I wasn't able to run tests from the testsuite without some changes, because some of C files don't end with a newline. I decided not to fix it in the current PR. Does anybody have the same behavior?

Copy link
Contributor

@mniewrzal mniewrzal left a comment

Choose a reason for hiding this comment

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

Thank you very much for this contribution! Looks very good, just have one nitpick in tests.

I've added a feature that is available already in the uplink-cli, but it isn't presented here. Actually, I need it for uplink-nodejs, I'll propose PR there in a few days.

When this will be merged we can make a new release for uplink-c so you could use official release for uplink-nodejs

Also, I wasn't able to run tests from the testsuite without some changes, because some of C files don't end with a newline. I decided not to fix it in the current PR. Does anybody have the same behavior?

I didn't have anything like that but if you can please push those changes as a separate PR and we will take a look.

};

UplinkSharePrefix prefixes[] = {
{"alpha", ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need empty prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I was writing tests, I referred to this line as an example of sharing access.
So I used the same bucket name and the prefix to be consistent with the rest of the codebase.

As far as I understood there are some issues with prefix and because of it, the empty prefix was set.

Erikvv
Erikvv previously approved these changes Feb 15, 2022
Copy link
Contributor

@Erikvv Erikvv left a comment

Choose a reason for hiding this comment

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

Good enough for me

uplink_compat.h Outdated Show resolved Hide resolved
Copy link
Contributor

@mniewrzal mniewrzal left a comment

Choose a reason for hiding this comment

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

Great! Thank you once again for this patch.

@mniewrzal mniewrzal merged commit 5bd23bc into storj:main Feb 16, 2022
@TopperDEL
Copy link

If someone tags a new release, I could bring this extension into uplink.NET soon.

@mniewrzal
Copy link
Contributor

I will make release today

@ishmelev-sp
Copy link
Contributor Author

Great, I will publish PR for uplink.nodejs soon.

@mniewrzal
Copy link
Contributor

@ishmelev-sp @TopperDEL sorry it's taking a bit more time to release uplink-c because we want to include latest uplink version with some additional improvements

@TopperDEL
Copy link

No problem, take your time! I just need a tagged version to integrate new features and I simply wanted to be "up-to-date" with uplink.NET. Otherwise that features is not necessary for me currently. :)

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.

None yet

5 participants