Skip to content

Conversation

msolefonte
Copy link
Contributor

@msolefonte msolefonte commented Mar 3, 2019

THIS PR IS A CONTINUATION OF #342
Motive of this change is that I want to sign this PR as an Individual, and not involve my Organization in it. To achieve it, I did the same commits again but with my personal account.


This patch modifies the zone value from R2 to Z2 to respect the format and the explanation of the example.

zone value has been modified from R2 to Z2 to respect the format and the explanation

Signed-off-by: Marc Solé <[email protected]>
Signed-off-by: Marc Solé <[email protected]>
@msolefonte
Copy link
Contributor Author

msolefonte commented Mar 3, 2019

@saad-ali @jdef There is a problem with protobuf again. Don't know why, because I did the make clean all as James said to me. Excuse me for not being used to Go at all.

@saad-ali
Copy link
Member

saad-ali commented Mar 3, 2019

Confirmed @msolefonte has signed CLA
@msolefonte try running:

make clean all
make all

@msolefonte
Copy link
Contributor Author

If I execute make all I get the message Nothing to be done for 'all'. Take a look to the log.

For make clean all:

make -C lib/go clean
make[1]: Entering directory '/home/msole/Documents/code/go/spec/lib/go'
go clean -i ./...
rm -rf "csi.a" "csi/csi.pb.go" "csi/.build"
make[1]: Leaving directory '/home/msole/Documents/code/go/spec/lib/go'
awk '{ if (length > 72) print NR, $0 }' csi.proto | diff - /dev/null
make -C lib/go
make[1]: Entering directory '/home/msole/Documents/code/go/spec/lib/go'
(cd "/home/msole/go/src" && \
        /home/msole/Documents/code/go/spec/lib/go/./protoc -I/home/msole/go/src -I/home/msole/Documents/code/go/spec/lib/go/.protoc/include --go_out=plugins=grpc,Mgoogle/protobuf/descriptor.proto=github.com/golang/protobuf/protoc-gen-go/descriptor,Mgoogle/protobuf/wrappers.proto=github.com/golang/protobuf/ptypes/wrappers:"/home/msole/Documents/code/go/spec/lib/go/csi/.build" "github.com/container-storage-interface/spec/csi.proto")
diff "csi/csi.pb.go" "csi/.build/github.com/container-storage-interface/spec/csi.pb.go" > /dev/null 2>&1 || cp -f "csi/.build/github.com/container-storage-interface/spec/csi.pb.go" "csi/csi.pb.go"
go get -v -d ./...
go install ./csi
go build -o "csi.a" ./csi
make[1]: Leaving directory '/home/msole/Documents/code/go/spec/lib/go'
make -C lib/cxx
make[1]: Entering directory '/home/msole/Documents/code/go/spec/lib/cxx'
cxx bindings & validation
make[1]: Leaving directory '/home/msole/Documents/code/go/spec/lib/cxx'

For make all:

awk '{ if (length > 72) print NR, $0 }' csi.proto | diff - /dev/null
make -C lib/go
make[1]: Entering directory '/home/msole/Documents/code/go/spec/lib/go'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/home/msole/Documents/code/go/spec/lib/go'
make -C lib/cxx
make[1]: Entering directory '/home/msole/Documents/code/go/spec/lib/cxx'
cxx bindings & validation
make[1]: Leaving directory '/home/msole/Documents/code/go/spec/lib/cxx'

It is an updated fork, so I think that the Makefile should not be the problem. however, and to avoid errors, this is the content of the file:

all: build

CSI_SPEC := spec.md
CSI_PROTO := csi.proto

# This is the target for building the temporary CSI protobuf file.
#
# The temporary file is not versioned, and thus will always be
# built on Travis-CI.
$(CSI_PROTO).tmp: $(CSI_SPEC) Makefile
	echo "// Code generated by make; DO NOT EDIT." > "$@"
	cat $< | sed -n -e '/```protobuf$$/,/^```$$/ p' | sed '/^```/d' >> "$@"

# This is the target for building the CSI protobuf file.
#
# This target depends on its temp file, which is not versioned.
# Therefore when built on Travis-CI the temp file will always
# be built and trigger this target. On Travis-CI the temp file
# is compared with the real file, and if they differ the build
# will fail.
#
# Locally the temp file is simply copied over the real file.
$(CSI_PROTO): $(CSI_PROTO).tmp
ifeq (true,$(TRAVIS))
	diff "$@" "$?"
else
	diff "$@" "$?" > /dev/null 2>&1 || cp -f "$?" "$@"
endif

build: check

# If this is not running on Travis-CI then for sake of convenience
# go ahead and update the language bindings as well.
ifneq (true,$(TRAVIS))
build:
	$(MAKE) -C lib/go
	$(MAKE) -C lib/cxx
endif

clean:
	$(MAKE) -C lib/go $@

clobber: clean
	$(MAKE) -C lib/go $@
	rm -f $(CSI_PROTO) $(CSI_PROTO).tmp

# check generated files for violation of standards
check: $(CSI_PROTO)
	awk '{ if (length > 72) print NR, $$0 }' $? | diff - /dev/null

.PHONY: clean clobber check

Signed-off-by: Marc Solé <[email protected]>
@msolefonte
Copy link
Contributor Author

msolefonte commented Mar 4, 2019

Well, this time I have been able to regenerate protobuf (my fault), but there is a merge conflict because of a recent merge in master. I'm trying to solve it.

EDIT: This has bean a headache but It's finally solved. Thank you very much for your help and attention.

@saad-ali
Copy link
Member

saad-ali commented Mar 5, 2019

Thank you for the clean up. Merging.

@saad-ali saad-ali merged commit c751be1 into container-storage-interface:master Mar 5, 2019
@jdef
Copy link
Member

jdef commented Mar 6, 2019

Going forward, it would be really nice if commit chains like this were squashed before merging. This was a single character change that now has 5 commits in our master commit history.

@msolefonte
Copy link
Contributor Author

@jdef
Excuse me. I just wanted to report a little mistake, which escalated to this pull request. Because of that, I never thought through a developer point of view.

I'm going to have it in mind for future pull requests, if there is any.

@jdef
Copy link
Member

jdef commented Mar 6, 2019

@msolefonte your PR is appreciated, my comment wasn't necessarily aimed at you :) The intended audience was the spec owners.

While IC squashes are greatly appreciated just prior to merge (and we may even ask for it to be done), ultimately the owners that merge the PR should ensure the commit chain is suitable for merge. Also, GH has a feature that lets owners "squash and merge" to avoid this kind of thing - it only takes a few more seconds to do and results in a cleaner commit chain.

Thanks again for the correction you submitted.

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.

3 participants