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

Use app password, change setup method #1957

Merged
merged 1 commit into from
Oct 24, 2022
Merged

Use app password, change setup method #1957

merged 1 commit into from
Oct 24, 2022

Conversation

Grotax
Copy link
Member

@Grotax Grotax commented Oct 23, 2022

The app password is nice in local testing, since Nextcloud logs too much data if you use the user password. Setup_file and teardown_file are only executed once instead of running before and after every job.

@Grotax Grotax added the Skip-Changelog No changelog update is required, minor change label Oct 23, 2022
@Grotax
Copy link
Member Author

Grotax commented Oct 23, 2022

Hmm annoying locally it is running fine.
Will look at it again maybe I missed something
Maybe another sleep is needed after the deletion of the app password.

As the sleep also helped in the beginning maybe the machine is too slow.

@SMillerDev
Copy link
Contributor

SMillerDev commented Oct 23, 2022

Could you try and use the occ command instead?

Edit: nextcloud/server#24317

@Grotax
Copy link
Member Author

Grotax commented Oct 24, 2022

Yea I will take a look at it, it seems that this api is not a 100% stable at least on my dev setup.

I ran it a few times and also added more sleep commands hoping that it would help.
But it turns out the behavior is a bit random. Sometimes the script requests a token, gets one and all is ok.
And sometimes something goes wrong and whatever is returned is not a valid token.

In manually running these actions I don't have that issue but I'm also slower than the bats script of course ...

The app password is nice in local testing, since
Nextcloud logs too much data if you use the user password.
Setup_file and teardown_file are only executed once instead
of running before and after every job.

Use bats from npm as ubuntu's version is old.

Switch to occ for app password

Signed-off-by: Benjamin Brahmer <info@b-brahmer.de>
@Grotax
Copy link
Member Author

Grotax commented Oct 24, 2022

Seems to work reliable now :) The occ command is apparently more stable for this situation. I left the cleanup command via the api as it seems like occ can't do that. And if it works it is nicer for a local setup not to have thousands of tokens.
If it doesn't works sometimes I can live with that.

For actions it doesn't matter of course since it always has a fresh db.

@@ -59,7 +59,7 @@ jobs:
coverage: none

- name: Setup BATS & httpie
run: sudo apt-get install -y bats httpie
run: sudo apt-get install -y httpie && npm install -g bats@1.7.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, why use npm for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the Ubuntu package has like 1.1 in their repo slightly newer on new versions.
There is also no backports. For the basic stuff it will work but of course there are details that are different.

I can't work with that though latest is 1.8 but I didn't test that yet on my machine since it didn't land in manjaro as of now.

npm is one of the projects official distribution ways (not sure why), I checked the repo for alternative install methods after I noticed that Ubuntu is hanging behind.

And since npm is available in the action image I think this is the best way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will probably in the future also add a requires bats version X line to the bats files.

I have no time to debug issues because of bugs that were already fixed by the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's start like this then

@Grotax Grotax merged commit 2432d56 into master Oct 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the test/update branch October 24, 2022 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip-Changelog No changelog update is required, minor change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants