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.

Sign up for GitHub

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

Jump to bottom

export pusher name ENV variable #316 #317

Merged
merged 1 commit into from
Dec 5, 2016
Merged

export pusher name ENV variable #316 #317

merged 1 commit into from
Dec 5, 2016

Conversation

afdev82
Copy link
Contributor

@afdev82 afdev82 commented Nov 30, 2016

This change will export the name of the pusher as an environment variable in order to use it in custom hooks.

@strk
Copy link
Member

strk commented Nov 30, 2016

Is there a document about available env variables that should be updated too ?

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 30, 2016
@tboerger tboerger added this to the 1.0.0 milestone Nov 30, 2016
tboerger
tboerger reviewed Nov 30, 2016
View reviewed changes
cmd/serve.go
@@ -256,6 +256,8 @@ func runServ(c *cli.Context) error {
}
}

os.Setenv("pusherName", user.Name)
Copy link
Member

Choose a reason for hiding this comment

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

I know the uuid is not done like that, but normally env variables are uppercase separated by underscores

@tboerger
Copy link
Member

tboerger commented Dec 1, 2016

The scope of these vars is so limited that I don't think we need to prefix them

@strk
Copy link
Member

strk commented Dec 1, 2016 via email

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 1, 2016
@afdev82
Copy link
Contributor Author

afdev82 commented Dec 1, 2016

@strk For what it's worth, I agree with you. 👍

@bkcsoft
Copy link
Member

bkcsoft commented Dec 2, 2016

@tboerger I agree with the others. The scope may be limited now, but they might expand in the future so might as well prefix them from the get-go.

@bkcsoft
Copy link
Member

bkcsoft commented Dec 2, 2016

@afdev82 Please note that Gitea also shoots git-hooks for pushes over HTTP, add the variable in the approriate place for that as well ;)

@tboerger
Copy link
Member

tboerger commented Dec 2, 2016 •
edited
Loading

Than we should also rename the uuid env variable and note that as a minor breaking change

@afdev82
Copy link
Contributor Author

afdev82 commented Dec 2, 2016

@bkcsoft Ok, first I have to figure out where :P
I don't know anything about GO and this project, I only identified the point where the other variable was added, and I simply copied the line. But I wanted to contribute, so I will figure out somehow.
@tboerger If it's fine to do in this PR, I will do it.

@strk strk mentioned this pull request Dec 2, 2016
Closed
@tboerger
Copy link
Member

tboerger commented Dec 2, 2016

@tboerger If it's fine to do in this PR, I will do it.

I'm ok with that.

@bkcsoft bkcsoft modified the milestones: 1.1.0, 1.0.0 Dec 3, 2016
@strk
Copy link
Member

strk commented Dec 4, 2016

LGTM - and no problem for me to put in 1.0.0

@tboerger
Copy link
Member

tboerger commented Dec 4, 2016

We should also export GITEA_UUID but keep the old env variable for backward compatibility

@tboerger tboerger added the type/enhancement An improvement of existing functionality label Dec 4, 2016
@tboerger
Copy link
Member

tboerger commented Dec 4, 2016

Beside my above comment LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 4, 2016
@afdev82
Copy link
Contributor Author

afdev82 commented Dec 4, 2016

Currently I'm still working on it, to export the other variable too and to test if I have actually the env variables, maybe I'm testing it in the wrong way. Could someone confirm that the variable is exported?

@tboerger
Copy link
Member

tboerger commented Dec 4, 2016

I'm still thinking about the old uuid env variable. Do we really need it? Does it really hurt if we just drop it and we mention it on the CHANGELOG on release?

@bkcsoft
Copy link
Member

bkcsoft commented Dec 4, 2016

@tboerger I've kept depricated API endpoints in my fixes and just added a note, and IMO that is good practice (keeping depricated things for a few minors)

About that, does anyone know where uuid is used? and what it does? 😛

@bkcsoft
Copy link
Member

bkcsoft commented Dec 4, 2016

@afdev82 I say keep the old value in-case someone uses it, but change the usage in cmd/update.go to use GITEA_UUID (line 52)

@tboerger
Copy link
Member

tboerger commented Dec 4, 2016

@afdev82 would you search for the uuid usage and replace it there as well? Than nobody else needs to do that and we can merge the PR.

@afdev82
Fixes #316
ba8cc55 
Export Pusher name as GITEA_PUSHER_NAME env variable
Export also GITEA_UUID, but keep the uuid env variable for backward compatibility

export pusher name ENV variable #316

change env variable prefix to GITEA_

Signed-off-by: Antonio Facciolo <afdev82@gmail.com>

Export also GITEA_UUID #316

Keep uuid env variable for backward compatibility
@bkcsoft
Copy link
Member

bkcsoft commented Dec 5, 2016

@tboerger that it now fixed

LGTM

@bkcsoft bkcsoft merged commit 947d2ee into go-gitea:master Dec 5, 2016
@tboerger tboerger modified the milestones: 1.0.0, 1.1.0 Dec 5, 2016
@Bwko Bwko mentioned this pull request Dec 6, 2016
Merged
@afdev82 afdev82 deleted the export_pusher_name branch November 10, 2017 08:11
@m47730 m47730 mentioned this pull request Jan 19, 2018
Closed
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tboerger tboerger tboerger left review comments

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@afdev82 @strk @tboerger @bkcsoft