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

adds ability to specify user and password via env variables #5980

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

adds ability to specify user and password via env variables #5980

wants to merge 7 commits into from
+12 −1

Conversation

internet-memme
Copy link

Proposal for issue #5875

It adds the ability to specify username and password in '--non-interactive' via $NC_USER and $NC_PASSWORD. This is the last option in the chain of username/password lookups.
It also updates the documentation accordingly.

claucambra
claucambra requested changes Aug 21, 2023
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Some style-related comments

src/cmd/cmd.cpp Outdated Show resolved Hide resolved
src/cmd/cmd.cpp Outdated Show resolved Hide resolved
claucambra
claucambra reviewed Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Thanks for applying the changes, looks good now! Could you remove the merge commits? After this it should be mergeable

@internet-memme
Copy link
Author

ok I think the merge should be reverted

@claucambra
Copy link
Collaborator

ok I think the merge should be reverted

I still see several merge commits

@internet-memme
Copy link
Author

I am not quite sure what I am supposed to do. Which commits exactly should I (revert?).

@claucambra
Copy link
Collaborator

I am not quite sure what I am supposed to do. Which commits exactly should I (revert?).

You should do an interactive rebase on master and drop the following commits:

You should also squash the remaining commits

@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5980-240c0ce7c297cd4c1e4ebf6d0a7f0365f50a33c4-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

claucambra
claucambra approved these changes Oct 1, 2023
View reviewed changes
Copy link
Collaborator

@claucambra claucambra left a comment

Choose a reason for hiding this comment

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

Changes looks ok, will merge as a single squashed commit

@claucambra claucambra enabled auto-merge (squash) October 1, 2023 12:14
@internet-memme
Copy link
Author

Oh thank you for doing the rebase :)

@codecov
Copy link

codecov bot commented Oct 1, 2023 •
edited
Loading

Codecov Report

Merging #5980 (2f7d468) into master (e30decc) will decrease coverage by 0.42%.
Report is 37 commits behind head on master.
The diff coverage is n/a.

❗ Current head 2f7d468 differs from pull request most recent head d3e7ed7. Consider uploading reports for the commit d3e7ed7 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5980      +/-   ##
==========================================
- Coverage   60.74%   60.32%   -0.42%     
==========================================
  Files         145      145              
  Lines       18841    18800      -41     
==========================================
- Hits        11445    11342     -103     
- Misses       7396     7458      +62

see 28 files with indirect coverage changes

@joshtrichards
Copy link
Member

joshtrichards commented Nov 27, 2023 •
edited
Loading

I suggest we set DCO to pass on this one - it's only failing on the @claucambra's revert merge branch single commit - unless someone has an objection.

@joshtrichards joshtrichards linked an issue Nov 27, 2023 that may be closed by this pull request
Provide user and password to nextcloudcmd using environment variables #5875
Open
@mgallien mgallien disabled auto-merge December 5, 2023 07:53
@nextcloud-desktop-bot
Copy link

AppImage file: nextcloud-PR-5980-d3e7ed7391c0a4431e036bb598a2a13c06107c2b-x86_64.AppImage

To test this change/fix you can simply download above AppImage file and test it.

Please make sure to quit your existing Nextcloud app and backup your data.

bonni and others added 7 commits August 18, 2024 18:17
@joshtrichards
reads env vars in --non-interactive
77cfc4b 
Signed-off-by: bonni <[email protected]>
@joshtrichards
makes control flow more obvious
0f5a0cc 
Signed-off-by: bonni <[email protected]>
@joshtrichards
adds documentation
91140b4 
Signed-off-by: bonni <[email protected]>
@joshtrichards
fixes typo
eb6e7da 
Signed-off-by: bonni <[email protected]>
@internet-memme @claucambra @joshtrichards
Update src/cmd/cmd.cpp
7022216 
Co-authored-by: Claudio Cambra <[email protected]>
Signed-off-by: internet-memme <[email protected]>
@internet-memme @claucambra @joshtrichards
Update src/cmd/cmd.cpp
30867a4 
Co-authored-by: Claudio Cambra <[email protected]>
Signed-off-by: internet-memme <[email protected]>
@joshtrichards
Revert "Merge branch 'cmd-env-vars' of github.com:internet-memme/nc-d…
f42ce09 
…esktop into cmd-env-vars"

This reverts commit e2e3c92, reversing
changes made to def7b65.
@joshtrichards joshtrichards added the enhancement enhancement of a already implemented feature/code label Aug 18, 2024
@mgallien
Copy link
Collaborator

@internet-memme sorry for the long delay
are you still interested in getting this PR merged ?
could you rebase and fix conflicts ?

@Rello Rello added this to the 3.16.0 milestone Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@claucambra claucambra claucambra approved these changes

Assignees

@claucambra claucambra

Labels
4. to release enhancement enhancement of a already implemented feature/code feature: 🔳 nextcloudcmd
Projects
None yet
Milestone
3.16.0
Development

Successfully merging this pull request may close these issues.

Provide user and password to nextcloudcmd using environment variables
6 participants
@internet-memme @claucambra @nextcloud-desktop-bot @joshtrichards @mgallien @Rello