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

Feature/noid/more remote testing #6663

Merged
merged 7 commits into from
Feb 17, 2022

Conversation

nickvergessen
Copy link
Member

No description provided.

@nickvergessen
Copy link
Member Author

Still not fully done, but another step taken and since it changes non-testing files I'd like to merge it to avoid future conflicts

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

But... I would like to remove the integration test from the second commit, as it does not work and I found it misleading when doing the review, and move the third commit to the end, as it depends on its following commits to work. Yes, you hate me, I know :-P

@@ -18,7 +24,13 @@ ${ROOT_DIR}/occ app:enable spreed || exit 1
${ROOT_DIR}/occ app:enable spreedcheats || exit 1
${ROOT_DIR}/occ app:list | grep spreed

# Disable bruteforce protection because the integration tests do trigger them
${ROOT_DIR}/occ config:system:set auth.bruteforce.protection.enabled --value false --type bool
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed in Talk tests too?

Copy link
Member Author

@nickvergessen nickvergessen Feb 15, 2022

Choose a reason for hiding this comment

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

Soon there will be bruteforce protection being added to the endpoint, yes.

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

I would still split the fourth commit and squash the related changes with the third... but ok, I will settle with this :-P

@nickvergessen
Copy link
Member Author

Drone conflict after #6730

Rebased

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@nickvergessen nickvergessen merged commit 7e57def into master Feb 17, 2022
@nickvergessen nickvergessen deleted the feature/noid/more-remote-testing branch February 17, 2022 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants