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

fix(test): skip artifact private repo test. Fixes: #8953 #9838

Merged
merged 1 commit into from Nov 2, 2022

Conversation

ibotdotout
Copy link
Contributor

@ibotdotout ibotdotout commented Oct 16, 2022

Fixes #8953

Introduced SKIP_PRIVATE_REPO environment variable to skip TestGitArtifactDriver_Load PrivateRepo test that some contributors lack of private repo access.

Update test to skip TestGitArtifactDriver_Load PrivateRepo.


Please do not open a pull request until you have checked ALL of these:

  • Create the PR as draft .
  • Run make pre-commit -B to fix codegen and lint problems.
  • Sign-off your commits (otherwise the DCO check will fail).
  • Use a conventional commit message (otherwise the commit message check will fail).
  • "Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
  • Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
  • Github checks are green.
  • Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

@ibotdotout ibotdotout marked this pull request as ready for review October 16, 2022 09:28
@ibotdotout ibotdotout changed the title refactor(test): mitigate test fail because of lack private repo access. Fixed: #8953 refactor(test): mitigate test fail because of lack private repo access. Fixes: #8953 Oct 16, 2022
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

There are already environment variables that controls this test

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

.

@ibotdotout ibotdotout changed the title refactor(test): mitigate test fail because of lack private repo access. Fixes: #8953 docs: add CI=true make test on docs running locally and git hook. Fixes: #8953 Oct 17, 2022
@ibotdotout
Copy link
Contributor Author

There are already environment variables that controls this test

I see then how about just update document and git hook pre-commit message.

Let me know if you have another suggestion.

@@ -199,6 +199,7 @@ Before you commit code and raise a PR, always run:

```bash
make pre-commit -B
CI=true make test
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong? CI should be false/unset unless on CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, just try to skip the test.

So, updated to skip it directly as you mentioned in #8953.

@ibotdotout ibotdotout changed the title docs: add CI=true make test on docs running locally and git hook. Fixes: #8953 fix(test): skip artifact private repo test. Fixes: #8953 Oct 18, 2022
@ibotdotout ibotdotout requested review from alexec and removed request for terrytangyuan October 18, 2022 14:28
@@ -25,6 +25,10 @@ func TestGitArtifactDriver_Load(t *testing.T) {
assert.DirExists(t, path)
})
t.Run("PrivateRepo", func(t *testing.T) {

// TODO: temp - skip private repo test for everyone
Copy link
Member

Choose a reason for hiding this comment

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

why do you want to skip this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @alexec mentioned here
#8953 (comment)

@alexec alexec enabled auto-merge (squash) October 29, 2022 15:46
Signed-off-by: ibotdotout <tkroputa@gmail.com>
auto-merge was automatically disabled October 31, 2022 04:26

Head branch was pushed to by a user without write access

@ibotdotout ibotdotout requested review from terrytangyuan and sarabala1979 and removed request for terrytangyuan October 31, 2022 06:09
@ibotdotout
Copy link
Contributor Author

ibotdotout commented Oct 31, 2022

Could you help to merge ?
I rebased and pushed to re-trigger e2e that failed and the auto-merge is gone.

@terrytangyuan terrytangyuan enabled auto-merge (squash) November 2, 2022 15:36
@terrytangyuan terrytangyuan merged commit a5b31b3 into argoproj:master Nov 2, 2022
juchaosong pushed a commit to juchaosong/argo-workflows that referenced this pull request Nov 3, 2022
…oproj#9838)

Signed-off-by: ibotdotout <tkroputa@gmail.com>
Signed-off-by: juchao <juchao@coscene.io>
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.

Unable to run artifact private git test
4 participants