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

User Agent and babun check issues #139

Merged
merged 5 commits into from May 26, 2014
Merged

User Agent and babun check issues #139

merged 5 commits into from May 26, 2014

Conversation

dogrizz
Copy link

@dogrizz dogrizz commented May 20, 2014

Hi
This is a fix for issue #138
I also took liberty of adding vim temp files to .gitignore.
I don't know if this is a best solution. It is something like a duct-tape fix more than anything.
But this helps when working behind draconian proxy.

Best regards
Dominik

@dogrizz dogrizz mentioned this pull request May 20, 2014
@tombujok
Copy link
Contributor

Awesome! Could you make two small fixes and resubmit?:

  • change -k to --insecure (so that it's more readable)
  • take the user agent from the $USER_AGENT env var (it's set in babun.rc, people may override it in .babunrc -> it's for cases where the firewall enforces certain browsers, only IE 9 for example)

Thanks!

@dogrizz
Copy link
Author

dogrizz commented May 20, 2014

Sure. I will get to it later tonight.

@dogrizz
Copy link
Author

dogrizz commented May 20, 2014

I looked a bit into your code and found something interesting.
You are aliasing curl to execute with User agent in babun.rc.
So why it doesn't work at all on my machine like that?
Maybe my installation can't read that file?
Because my proxy is not restricting value of the user agent at all.
I will have to check that tommorow before I update this.

@tombujok
Copy link
Contributor

Ok - perfect. Please have a look.
I think the reason is that aliases are not used in shell scripts (non-interactive shells), thus it has to be added there by hand...
pact would have to be checked as well... (sic!)

@dogrizz
Copy link
Author

dogrizz commented May 21, 2014

Ok. So it seems my proxy had issues with that specific user agent ;)
But babun was not taking into account what I was setting in .babunrc as value of agent was evaluated upon execution of babun.rc and not on execution of command.

Do we have some tests for this?
The easy way to check is:

{ plugins } master » type curl 
curl is an alias for curl -A "$USER_AGENT"

If this has set user agent instead of variable then settings from .babunrc are ignored.

And this should return our set agent. (a bit messy but works).

curl http://www.whatsmyuseragent.com/ | grep "body_lbUserAgent" 

Pact works.

@dogrizz dogrizz changed the title Fix for issue #138 User Agent issues May 21, 2014
@dogrizz dogrizz changed the title User Agent issues User Agent and babun check issues May 21, 2014
@tombujok
Copy link
Contributor

Aha - but when you change .babunrc you have to execute source .babunrc, then it sohuld work. The same if you restart babun. Do you agree?
Your case was that you modified .babunrc and used curl immediately, or am I missing sth?

@dogrizz
Copy link
Author

dogrizz commented May 21, 2014

Nope. Even after restart variable is not read.
You can check it with:

type curl

As i understand the issue goes like that:

  • First babun.rc is read
  • USER_AGENT set
  • aliasing curl evaluates USER_AGENT so it sets to curl -A "whatever was in user agent"
  • .babunrc is read
  • USER_AGENT set to what user wants
  • curl and wget alias stays as it was because USER_AGENT was already evaluated

@tombujok
Copy link
Contributor

I see the problem - it's a chicken-egg problem.
The aliases are evaluated before .babunrc, so that the user can overwrite them.
If I evaluate the aliases after user's .babunrc, some of the user's aliases may be overwritten...
What do you suggest?

Thanks for your engagement in other tickets as well - it's awesome!

@dogrizz
Copy link
Author

dogrizz commented May 21, 2014

So now it won't evaluate USER_AGENT in alias till you execute it.
So alias will be set to: curl -A "$USER_AGENT" as seen in type result.
But when executed $USER_AGENT will be set to what is in bash.rc or
what user set in .bashrc. So it will work now as intended.

I don't see other aliases in babun.rc that might be affected.
But as I said in other issue the CYGWIN value might be evaluated eagerly somewhere.

@tombujok
Copy link
Contributor

Ah, yes, that's great! - I've learnt something new 👍

@tombujok
Copy link
Contributor

So you've also checked that the user agent will be used by pact as well?

@dogrizz
Copy link
Author

dogrizz commented May 21, 2014

It works in pact because it evaluates USER_AGENT in the script and adds to wget calls manually.
It will work with USER_AGENT even if user sets his own alias for wget.

@dogrizz
Copy link
Author

dogrizz commented May 26, 2014

You want me to check something else?

@tombujok
Copy link
Contributor

No, your changes are perfect.
I was fighting with the #74 bug over the weekend and did not manage to merge this one.

I think we need the insecure option here as well:
https://github.com/babun/babun/blob/master/babun-core/tools/check.sh

Actually, I have one question:
I was just wondering if the alias will be used at all in the case of curl - I've read once that the aliases are not used in non-interactive shells (bash scripts). Don't we have to add it by-hand as in pact (in wget)?

@dogrizz
Copy link
Author

dogrizz commented May 26, 2014

Yep. Looks like my cntlm user agent was doing work for me ;)
Rechecked and added to check.sh

@tombujok tombujok merged commit 26a37f8 into babun:master May 26, 2014
tombujok added a commit that referenced this pull request May 26, 2014
@tombujok
Copy link
Contributor

Merged manually, due to conflicts, but your commits are there -thx!

@dogrizz
Copy link
Author

dogrizz commented May 27, 2014

Yey first pull request ;) Glad to be of service

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.

None yet

2 participants