Skip to content
This repository has been archived by the owner on Jul 5, 2019. It is now read-only.

Add new protocol #65

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add new protocol #65

wants to merge 6 commits into from

Conversation

zhkufish
Copy link

@zhkufish zhkufish commented Jan 9, 2018

"auth_sha1_v4",
"auth_aes128_md5",
"auth_aes128_sha1"

.gitmodules Outdated
@@ -1,6 +1,6 @@
[submodule "Library/ShadowPath/ShadowPath/shadowsocks-libev"]
path = Library/ShadowPath/ShadowPath/shadowsocks-libev
url = [email protected]:icodesign/shadowsocks-libev.git
url = Library/ShadowPath/ShadowPath/shadowsocks-libev.git
Copy link
Owner

Choose a reason for hiding this comment

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

Something wrong here. It should be remote github url, not the local one.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, I will fix it.

logglyLogger.logFormatter = formatter
DDLog.add(logglyLogger)
// let logglyLogger = LogglyLogger() // Loggy Logger
// logglyLogger.logglyKey = "107d98a8-c275-4369-a881-b36d3564c9ef"
Copy link
Owner

@haxpor haxpor Jan 9, 2018

Choose a reason for hiding this comment

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

What happens here. I think this PR should not involve touching too many thing. LogglyLogger can be left intact, and code before changed grabs api key from Info.plist nicely, no need to hardcoded.

I think this part should not be commented or removed out.

What's the problem you're facing if not commenting those lines?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know to use LogglyLogger. I had register an account in "loggly.com". I use customer token replace the logglyKey. But it had a connection error when I run Potatso. So I remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Today, I try again, It seems normally.
Suspending, posting logs to Loggly
2018-01-10 11:35:15.647177+0800 Potatso[1576:769317] Posting to Loggly: {"rawlogmessage":

Copy link
Owner

Choose a reason for hiding this comment

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

Is it working before before you made change? InfoInternal will grab API key from Info.plist automatically.

Copy link
Owner

Choose a reason for hiding this comment

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

Anyway, in future I think we can remove it as developer will build their own app and change API key anyway.

HelpshiftCore.initialize(with: HelpshiftAll.sharedInstance())
HelpshiftCore.install(forApiKey: HELPSHIFT_KEY, domainName: HELPSHIFT_DOMAIN, appID: HELPSHIFT_ID)
}
// func configHelpShift() {
Copy link
Owner

Choose a reason for hiding this comment

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

I thought about removing HelpShift as well. I will see how we will proceed this i.e cherry pick commits, or else.

response.result.error?.log("Fail to update ruleset details")
}
}
// API.updateRuleSetListDetail(uuidsArray) { (response) in
Copy link
Owner

@haxpor haxpor Jan 9, 2018

Choose a reason for hiding this comment

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

Just note, if you don't need any code, just remove it, no need to comment them out like this.

But we should include code in CloudSetManager as for later we will do #62 .
So I think, better to leave code in CloudSetManager.swift intact, not remove them yet.

Copy link
Author

Choose a reason for hiding this comment

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

OK. Rarely used git. We use svn in out company. This is my first pull requests in github. it make some mistake.

@@ -14,7 +14,7 @@ + (NSString *) sharedGroupIdentifier {
// reverted back
// very curious why grabbing value from Info.plist will result in cannot connect to VPN all the time
// it needs to be fixed like this as always
return @"group.io.wasin.potatso";
return @"group.com.aiyuangong.Aiyuangong";
Copy link
Owner

@haxpor haxpor Jan 9, 2018

Choose a reason for hiding this comment

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

I'm wondering what's the best approach for future going forward to allow contributors to contribute that it needs to change domain in order to successfully build like this. Without making changes to repo.

If you have any idea for best approach to do that, feel free to let me know.

BTW: Can you build the project by not changing the bundle id, or group identifier on your machine?

Copy link
Author

Choose a reason for hiding this comment

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

No. I can't build without change bundle id. It seems to have to use paid apple developer when you use NetworkExtension.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, thank you for letting me know.
I will think about cleaner way not to pollute changes every time others push commit later.

"auth_sha1_v4",
"auth_aes128_md5",
"auth_aes128_sha1",
"auth_chain_a",
Copy link
Owner

@haxpor haxpor Jan 9, 2018

Choose a reason for hiding this comment

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

Can we remove these two lines for auth_chain_a and auth_chain_b as you said it doesn't support yet?

Copy link
Author

@zhkufish zhkufish Jan 10, 2018

Choose a reason for hiding this comment

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

Sorry. I forgot to remove it in this file.

@haxpor
Copy link
Owner

haxpor commented Jan 9, 2018

Please ignore result from travis-ci, I'm not done with it yet.

- CallbackURLKit (1.0.0):
- CallbackURLKit/Core (= 1.0.0)
- CallbackURLKit/Core (1.0.0)
- Cartography (1.0.1)
- Cartography (3.0.1)
Copy link
Owner

Choose a reason for hiding this comment

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

Are you sure Cartogrphy 3.0.1 works with Potatso?
Did you try cleaning up the project before building? Thanks

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I had cleaning up before. no errors.

Copy link
Owner

@haxpor haxpor left a comment

Choose a reason for hiding this comment

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

More feedback regarding updating of dependencies.

@zhkufish
Copy link
Author

Please ignore result from travis-ci, I'm not done with it yet. What's this? Where used?

@haxpor
Copy link
Owner

haxpor commented Jan 10, 2018

I mean

screen shot 2018-01-10 at 11 57 30 am

no need to worry about it :)

@zhkufish
Copy link
Author

I had fix smoe mistake. But I can't create new pull requests. It remind this warning, "Can’t automatically merge. Don’t worry, you can still create the pull request." But I can't find any create pull reqest button

@haxpor
Copy link
Owner

haxpor commented Jan 10, 2018

It's ok, no need to create a new PR. Your new commits are updated with this same PR.
I'll test it out later. Thank you for your contribution!

// logglyLogger.logFormatter = formatter
// DDLog.add(logglyLogger)
let logglyLogger = LogglyLogger() // Loggy Logger
logglyLogger.logglyKey = "107d98a8-c275-4369-a881-b36d3564c9ef"
Copy link
Owner

@haxpor haxpor Jan 10, 2018

Choose a reason for hiding this comment

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

Would you please change back to this logglyLogger.logglyKey = InfoInternal.shared.getLogglyAPIKey()?

Copy link
Author

Choose a reason for hiding this comment

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

ok.

@@ -37,7 +37,7 @@ class AppInitializer: NSObject, AppLifeCycleProtocol {
DDLog.add(fileLogger)

let logglyLogger = LogglyLogger() // Loggy Logger
logglyLogger.logglyKey = InfoInternal.shared.getLogglyAPIKey()
logglyLogger.logglyKey = "107d98a8-c275-4369-a881-b36d3564c9ef"
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please revert back to this logglyLogger.logglyKey = InfoInternal.shared.getLogglyAPIKey()?

"auth_sha1_v4",
"auth_aes128_md5",
"auth_aes128_sha1",
// "auth_chain_a",
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this line and below it.

getLogglyAPIKey from info.plist
@pengxianhe
Copy link

can this support protocol_param?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants