Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

More code and script change to enable vFile CI #1886

Merged
merged 40 commits into from
Oct 25, 2017
Merged

More code and script change to enable vFile CI #1886

merged 40 commits into from
Oct 25, 2017

Conversation

luomiao
Copy link
Contributor

@luomiao luomiao commented Sep 14, 2017 •
edited
Loading

@ashahi1
Please generate the key. Thanks!

Update:
Due to the slowness of the testbed, CI testing exposed a few problems in the code.
Besides adding scripts change, this PR also includes several fixes:

  1. Change the waiting for file server path mounting from one time waiting to a timeout waiting. -> Fixes: [vFile] vFile operation fails with Error: exit status 255 if swarm nodes are slow #1941 Failed to mount vFile volume. Error: exit status 255 on Swarm master #1816
  2. When some of the nodes in the swarm network failed/rejected for an assigned docker service task, the status of this task will stay in task lists if polling information according to service ID. Current code will return false as long as one task is failed/rejected. However the correct handling should return true as long as one task is running, since we only need one running container for the file server service. Fixes: Enable CI Test for vFile plugin: resolving current bugs #1911

@ashahi1
Copy link
Contributor

ashahi1 commented Sep 15, 2017

CI run has started with the new key.

shuklanirdesh82
shuklanirdesh82 previously requested changes Sep 18, 2017
View reviewed changes
.drone.yml Outdated
@@ -49,6 +49,7 @@ build:
- go get github.com/golang/lint/golint
- go get -u gopkg.in/check.v1
- make -s build
- make -s build-vfile
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to include the target into the build target it self

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we include vFile also into build target, all the following development for vDVS will suffer waiting for building vfile too. The build of vfile and vdvs are totally separate from each other. So I think we should have them as different build target.

Copy link
Contributor

Choose a reason for hiding this comment

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

building vFile binary wont take much time it is still OK to build binary as part of build having it said building and pushing plugin should be a separate target and I think it is there. Am I missing anything here?

@luomiao luomiao force-pushed the testing-update branch 3 times, most recently from 4735e04 to 034b10d Compare September 26, 2017 00:24
@luomiao luomiao force-pushed the testing-update branch 3 times, most recently from 369469c to 1211316 Compare September 29, 2017 21:22
@luomiao luomiao mentioned this pull request Oct 5, 2017
Closed
@luomiao luomiao force-pushed the testing-update branch 2 times, most recently from 5cea0fa to faeb905 Compare October 20, 2017 00:54
@luomiao luomiao changed the title More scripts change to enable vFile CI More code and script change to enable vFile CI Oct 20, 2017
@luomiao
Copy link
Contributor Author

luomiao commented Oct 20, 2017

@shuklanirdesh82
The updated PR has addressed Nirdesh's comment to include vfile as part of build target.
Since Nirdesh is PTO now, I will just dismiss this review since it's already cleared.

@luomiao luomiao dismissed shuklanirdesh82’s stale review October 20, 2017 21:02

This has been addressed in the following commits.

@luomiao
Copy link
Contributor Author

luomiao commented Oct 24, 2017

@lipingxue
since there are some changes to vfile code too, could you also please review those changes?
Thanks.

@lipingxue
Copy link
Contributor

@luomiao OK. I will review it.

lipingxue
lipingxue approved these changes Oct 24, 2017
View reviewed changes
Copy link
Contributor

@lipingxue lipingxue left a comment

Choose a reason for hiding this comment

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

Change in vFile related change looks good to me.

ashahi1
ashahi1 approved these changes Oct 25, 2017
View reviewed changes
Copy link
Contributor

@ashahi1 ashahi1 left a comment

Choose a reason for hiding this comment

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

Changes related to CI look alright to me.

@luomiao luomiao merged commit 8a6951e into vmware-archive:master Oct 25, 2017
This was referenced Oct 27, 2017
Closed
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shuklanirdesh82 shuklanirdesh82 shuklanirdesh82 left review comments

@ashahi1 ashahi1 ashahi1 approved these changes

@lipingxue lipingxue lipingxue approved these changes

Assignees
No one assigned
Labels
cla-not-required
Projects
None yet
Milestone
No milestone
5 participants
@luomiao @ashahi1 @lipingxue @shuklanirdesh82 @vmwclabot