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

I have added logic to lookup os.environ 'URL_PREFIX' if exists use it instead of the DEFAULT_PREFIX #108

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

Conversation

myselfram
Copy link

@notjames @joejulian @ant31
if my understanding is correct this should work, please review and help me merge this code.

Regards,
Ram

@myselfram
Copy link
Author

@notjames @joejulian @ant31
if my understanding is correct this should work, please review and help me merge this code.

Regards,
Ram

Copy link
Contributor

@notjames notjames left a comment

Choose a reason for hiding this comment

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

Lgtm, but see my comment

@myselfram
Copy link
Author

@notjames @joejulian @ant31 @steved @philips @polvi @mtalbot

What else do I need to do to be able to merge this code change?
Please help.

Regards,

Ram Srinivasan

@myselfram
Copy link
Author

@notjames @joejulian @ant31 @steved @philips @polvi @mtalbot

This is frustrating I was hoping someone would respond.

Please help me with this change request.

Regards,

Ram Srinivsan

@notjames
Copy link
Contributor

@myselfram I'm not sure how much experience you have with github. If I may opine, you're kind of new to it. Taking a look at this project seems to indicate that it's probably a dead project considering no commits/PRs seem to have been done in over 5 years. Moreover, I thought this was a dead project as I thought appr was no longer necessary (I could be wrong there). Nevertheless, you've managed to get some attention of people in this project, but not the right people and it's possible you're not going to get the attention of a maintainer given the length of time this project has been updated. You might be better off forking this project and using your own fork for your needs rather than waiting for someone to commit/merge this PR.

@joejulian
Copy link

Also, in the open source world, patience is a must. You pinged me while I was on a 2 week vacation. No service, just trees, water, sea otters, and orcas. I'm not a contributor to this repo so I really have no idea what this PR is about or why it should or shouldn't be included and I really don't need negativity on my return from such a wonderful time away.

Also, as Jim says, this hasn't seen a commit in years. That's not to say that the owner might not accept it, but it would require that they have the time to even take a look. Most folks have day jobs and only maintain these old repos in their spare time.

Give people a break. Stand in their shoes.

@notjames
Copy link
Contributor

notjames commented Jul 6, 2023

Also, in the open source world, patience is a must. You pinged me while I was on a 2 week vacation. No service, just trees, water, sea otters, and orcas. I'm not a contributor to this repo so I really have no idea what this PR is about or why it should or shouldn't be included and I really don't need negativity on my return from such a wonderful time away.

Also, as Jim says, this hasn't seen a commit in years. That's not to say that the owner might not accept it, but it would require that they have the time to even take a look. Most folks have day jobs and only maintain these old repos in their spare time.

Give people a break. Stand in their shoes.

Joe, glad to hear you had a good vacation. Thanks for chiming in.

@@ -57,6 +57,8 @@ def _configure_endpoint(self, endpoint):
else:
scheme = "https://"
endpoint = scheme + endpoint
if 'URL_PREFIX' in os.environ:
Copy link
Contributor

Choose a reason for hiding this comment

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

please name it: APPR_URL_PREFIX and it should be good to merge

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.

4 participants