-
Notifications
You must be signed in to change notification settings - Fork 12
Make company configurable as far as possible #8
base: master
Are you sure you want to change the base?
Conversation
setup.sh
Outdated
| echo -n 'Please enter your GitHub Enterprise username and press [ENTER]: ' | ||
| else | ||
| echo -n "Please enter your Autodesk username and press [ENTER] (empty for \"$STORED_GITHUB_ENTERPRISE_ACCOUNT\"): " | ||
| echo -n "Please enter your GitHub Enterprise username and press [ENTER] (empty for \"$STORED_GITHUB_ENTERPRISE_ACCOUNT\"): " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of our engineers confuse GitHub Enterprise with github.com. Therefore I used the company name here. Maybe we should make this an enterprise.config setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree.
| # [jokram] Why is the checkout of a new branch adsk-setup necessary? The reset --hard will overwrite it, or? | ||
| # Or is it just to mark that this was the previous version? | ||
| git --git-dir="$KIT_PATH/.git" --work-tree="$KIT_PATH" checkout --quiet -B $KIT_ID-setup && \ | ||
| git --git-dir="$KIT_PATH/.git" --work-tree="$KIT_PATH" reset --quiet --hard origin/$BRANCH && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkout is necessary to make sure we are on the "adsk-setup" branch. If you develop enterprise-config then you might be on branch X. If you run it then it would hard reset X to the latest production state and you would "lose" your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now I got it:
git reset --hard origin/production will modify the current branch pointer (which might be branch-X) to also point to origin/production. And by this the branch-X would have been modified. So git checkout -B adsk-setup is just to ensure that you have temp. branch which can be modified by the reset command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why not just: git checkout -B adsk-setup origin/production?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That call can fail if you have local uncommitted changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. Next try: git checkout -f -B adsk-setup origin/production
Documentation for -f says:
When switching branches, proceed even if the index or the working tree differs from HEAD. This is used to throw away local changes.
enterprise.constants
Outdated
|
|
||
|
|
||
| # Add the protocol used to access GitHub server: http or https | ||
| GITHUB_PROTOCOL='https' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should follow your naming convention from GHE_USER etc. here and call this GHE_HTTP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus, I would vote against using the term PROTOCOL because that could also mean "SSH"...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then maybe also for GITHUB_SERVER => GHE_SERVER
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/setup_helpers.sh
Outdated
| KIT_PATH=$(dirname "$0") | ||
| . "$KIT_PATH/../enterprise.constants" | ||
|
|
||
| PROTOCOL=$GITHUB_PROTOCOL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you define PROTOCOL here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would not be necessary, but I was thinking that the helpers normaly are independent from GitHub, so I wanted to avoid to mention in the helper functions no GITHUB_* variable.
What would you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this approach if it is a local variable in a function. But here PROTOCOL as well as GITHUB_PROTOCOL are global variables and therefore I would prefer to only use the latter.
|
@jokram : I haven't forgotten this PR. I am just super busy right now. This looks pretty good - I'll merge it ASAP. Are you blocked in any way? |
|
@larsxschneider: No problem. I'm still analyzing. |
|
OK. Sounds good! Don't hesitate to reach out to me if you run into any trouble with the |
|
Concerning the proxy you might be right. But this will take a bit longer to understand, but it might be worth to avoid problems in the field. I've created a DESIGN.md file to write down my analysis results. In addition this contains some design ideas you might have a look at (if the stress is decreasing a bit). But these ideas should be discussed first before continuing. Maybe it makes sense to cover this in a separate pull request or issue. |
Require latest Git and Git LFS versions
Tried to make the code independent from Autodesk. So it should only depend on
enterprice.constants.Exceptions:
config.include: Here I've not yet found a way to make this dependent on enterprise.constantsReadme.md: For another company the Readme should be adapted, because it is delivered to end usersImportant Notes:
So I've merged feature/configurable-protocol into this branch. Best is if you first check the other pull request Add GITHUB_PROTOCOL support (http or https) #7