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

Add test for 'ipfs config replace' #3073

Merged
merged 8 commits into from
Oct 4, 2016
Merged

Conversation

Kubuxu
Copy link
Member

@Kubuxu Kubuxu commented Aug 11, 2016

No description provided.

@Kubuxu Kubuxu added the need/review Needs a review label Aug 11, 2016
@whyrusleeping whyrusleeping added the status/in-progress In progress label Aug 18, 2016
@@ -81,8 +94,6 @@ test_config_cmd() {
test_cmp ident_exp ident_out
'

# SECURITY
# Those tests are here to prevent exposing the PrivKey on the network
Copy link
Member

Choose a reason for hiding this comment

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

whyd you remove this comment?

Copy link
Member

Choose a reason for hiding this comment

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

oh nvm, i see that you moved it up

@whyrusleeping
Copy link
Member

Can you maybe break that test up a little bit? maybe into a setup step, the command execution, and the output check?

@whyrusleeping whyrusleeping added need/author-input Needs input from the original author and removed need/review Needs a review labels Aug 24, 2016
@whyrusleeping
Copy link
Member

@Kubuxu any update here?

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 8, 2016

How does it look now?

@Kubuxu Kubuxu removed the need/author-input Needs input from the original author label Sep 8, 2016
@Kubuxu Kubuxu removed their assignment Sep 8, 2016
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@whyrusleeping
Copy link
Member

Much better, except you've angered the mighty travis CI.

@Kubuxu Kubuxu self-assigned this Sep 9, 2016
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 13, 2016

@whyrusleeping OSX/BSD sed is behaving quite strangely with -i flag and it looks like it will not support search and execute. Can I add gnu-sed as a requirement on OSX/BSD based systems?

//cc @chriscool

@chriscool
Copy link
Contributor

@Kubuxu I think it is better to just avoid using the -i option when using sed.
You can instead redirect into new files for example like:

    sed -e /PrivKey/d -e s/10GB/11GB/ newconfig.orig.json >newconfig.step.json &&
    sed -e '"'"'/PeerID/ { s/,$// } '"'"' newconfig.step.json >newconfig.json

@Kubuxu Kubuxu added status/ready Ready to be worked and removed status/in-progress In progress labels Sep 26, 2016
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@chriscool
Copy link
Contributor

It looks like there are still sed related problems:

expecting success: 

    cp "$IPFS_PATH/config" newconfig.json &&
    </dev/null sed -e /PrivKey/d -e s/10GB/11GB/ -i"~"  newconfig.json &&
    </dev/null sed -e '/PeerID/ { s/,$// } ' -i"~" newconfig.json

sed: 1: "/PeerID/ { s/,$// } 
": bad flag in substitute command: '}'
not ok 49 - setup for config replace test

License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu Kubuxu added status/in-progress In progress and removed status/ready Ready to be worked labels Sep 28, 2016
License: MIT
Signed-off-by: Jakub Sztandera <[email protected]>
@Kubuxu
Copy link
Member Author

Kubuxu commented Sep 28, 2016

It works, Shanress on OSX passed, I will squash everything.

@Kubuxu Kubuxu added the need/review Needs a review label Oct 3, 2016
@Kubuxu Kubuxu removed their assignment Oct 3, 2016
@whyrusleeping
Copy link
Member

LGTM, thanks @Kubuxu and @chriscool :)

@whyrusleeping whyrusleeping merged commit 5c212d3 into master Oct 4, 2016
@whyrusleeping whyrusleeping deleted the feat/test-config-replace branch October 4, 2016 00:29
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Oct 4, 2016
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants