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

Add Proposal of Refactor prepare_script #22

Merged
merged 2 commits into from Apr 24, 2019
Merged

Conversation

ninjadq
Copy link
Member

@ninjadq ninjadq commented Nov 22, 2018

propose to refactor prepare script

@steven-zou
Copy link
Contributor

@ninjadq

Please squash your commits and fix the DCO issue.

@ninjadq ninjadq force-pushed the ninjadq-patch-1 branch 8 times, most recently from eb9f477 to 1fa9b97 Compare January 4, 2019 06:12
@ninjadq ninjadq changed the title Create propose_to_refactor_prepare_script proposal to refactor prepare_script Jan 4, 2019
@ninjadq ninjadq changed the title proposal to refactor prepare_script Add Proposal of Refactor prepare_script Jan 4, 2019


4. Current config file is `harbor.cfg` which is configparser-style format. In order to describe more complex content, We need replace it using YAML format like `harbor.yml` . current config file may look like this.
```yaml
Copy link
Contributor

@reasonerjt reasonerjt Jan 4, 2019

Choose a reason for hiding this comment

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

Would it be possible to organize the yaml file in a more readable structure?
For example:

core:
    proxy:
        ....
registry:
    ......
    storage:
         ......
clair:
     ......
notary:
     .....

Additionally, shall we remove the "user settings" from the configuration file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree with you, the final config file should like this. But the first step is compatible with the previous configParser format.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments

@reasonerjt
Copy link
Contributor

Two other goals we should consider to achieve in this refactor:

  1. The default data volume /data should be configurable. Particularly, the secretkey should be moved away from the data volume. @ywk253100 knows more background.
  2. The sudo should be removed from entrypoints of the containers in Harbor.

@reasonerjt
Copy link
Contributor

We also need a solution for migration from old harbor.cfg to harbor.yaml
I think that's do-able following the way we did, refer to:
https://github.com/goharbor/harbor/tree/master/tools/migration/cfg

Copy link
Contributor

@steven-zou steven-zou left a comment

Choose a reason for hiding this comment

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

Please do changes.

├── Pipfile
├── Pipfile.lock
├── g.py
├── main.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding comments for each file to describe what does the file do.

1. ```
├── Pipfile
├── Pipfile.lock
├── g.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest moving all the none global variables out of the g.py as it's used to keep global variables. Or rename it to v.py/values.py.



4. Current config file is `harbor.cfg` which is configparser-style format. In order to describe more complex content, We need replace it using YAML format like `harbor.yml` . current config file may look like this.
```yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comments

| uaa_verify_cert | boolean |


5. Packaging all files and its denpendencies into a container. All these codes and changes is related to python, so we should using official python images as the base image in Dockerfile. But there is a issue that some action need super user to operate. like change the owner and group of an file, create files on host system. I have no proper method to handle this. Maybe mount the directory on host specified in config file to container can solve last question.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest adding an upgrading section to talk about how to upgrade config from old version to new version.


4. Current config file is `harbor.cfg` which is configparser-style format. In order to describe more complex content, We need replace it using YAML format like `harbor.yml` . current config file may look like this.
```yaml
## Configuration file of Harbor
Copy link
Contributor

Choose a reason for hiding this comment

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

Please append the property table copied from daojun's PR to let reviewers know what settings will be removed from harbor.yaml.

@nlowe
Copy link
Contributor

nlowe commented Jan 10, 2019

I know the existing prepare script is written in python. Have we considered re-writing it in go instead, and using go templates? It drops the dependency on Python at runtime and brings the utility closer in-line with the majority of the codebase.

As far as I can tell, python is only used in some test scripts and the CLI tool which is deprecated.

@ninjadq
Copy link
Member Author

ninjadq commented Jan 11, 2019

I know the existing prepare script is written in python. Have we considered re-writing it in go instead, and using go templates? It drops the dependency on Python at runtime and brings the utility closer in-line with the majority of the codebase.

As far as I can tell, python is only used in some test scripts and the CLI tool which is deprecated.

I think the effort of rewriting it into go and go template is large then refactor in python because we can reuse a lot of current codes and these codes are verified by time. In addition, we wanna put all the dependencies of prepare into a container, so that user does not care about the python dependencies anymore. go is excellent but from my knowledge, python is good at this kind scripts.

@ghost
Copy link

ghost commented Feb 1, 2019

I have some relatively broad thoughts on the topic of having to run a prepare script in the first place. I'm going to parse those thoughts and eventually submit a fleshed out proposal.

With that said, though, I do agree with @nlowe that having multiple components in disparate languages is something we should eventually aim to address. We should pick one lingua franca and stick with it as much as possible (within reason).

@ninjadq ninjadq force-pushed the ninjadq-patch-1 branch 2 times, most recently from 7f83531 to 2820575 Compare February 13, 2019 07:14
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 14, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 14, 2019
…are_script.md

Signed-off-by: Qian Deng <dengq@vmware.com>
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 21, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 21, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 22, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 22, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 26, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Feb 26, 2019
ninjadq added a commit to ninjadq/harbor that referenced this pull request Mar 11, 2019
@steven-zou
Copy link
Contributor

Per lazy consensus rule, merge proposal to main.

@steven-zou steven-zou merged commit bf8fc22 into master Apr 24, 2019
@steven-zou steven-zou deleted the ninjadq-patch-1 branch April 24, 2019 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants