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

Adding the ability to add SCMs through config repo #5756

Merged
merged 4 commits into from Feb 4, 2019
Merged

Conversation

ibnc
Copy link
Contributor

@ibnc ibnc commented Jan 22, 2019

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Jan 23, 2019

@tomzo Would you mind taking a look at what I've got thus far?

@maheshp maheshp added this to the Release 19.2.0 milestone Jan 24, 2019
@maheshp maheshp added this to In progress in 19.2.0 Jan 24, 2019
@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Jan 31, 2019

@tomzo would you mind taking another look at my PR now that I've made some changes related to your feedback?

@tomzo
Copy link
Member

@tomzo tomzo commented Feb 1, 2019

@ibnc looks OK, I left 2 small suggestions.
From my experience, it is good to test manually a few cases on development server. The most valuable would be:

  • add some SCMs in xml, then one in configrepo. Try to remove/add SCM in xml. Also through API.
  • push invalid SCM so that merge fails, Try to remove/add SCM in xml. Also through API. It should still work because of fallback merge.

ibnc added 2 commits Feb 1, 2019
* Changes to underlying logic as well
* test fixes and added tests
@arvindsv
Copy link
Member

@arvindsv arvindsv commented Feb 1, 2019

@ibnc Not sure, but I don't think this change should need a schema bump. Ideally, this shouldn't change the existing tags at all. I know that pipeline groups and even environments end up putting an extra tag in the config. I don't remember why.

Is that the reason the schema bump is needed? If so, can you explain the reason (and I'll try to remember this time)? Thank you!

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 1, 2019

@arvindsv It's so we can show something in the xml that isn't the full SCM config. I suspect environments and pipeline groups did this to create a visual distinction between locally and remotely configured items. This won't be needed after we remove the config xml page.

here's an example of what the schema change allows for:

<scms>
  <scm id="8e27f393-97e2-4572-983b-6219d9cf718a" name="wubba_lubba_dub_dub" />
</scms>

@tomzo
Copy link
Member

@tomzo tomzo commented Feb 1, 2019

For environments and pipeline groups, there exists an empty tag to represent the main xml into which we are merging partials. This was added by @jyotisingh when fixing merge algorithm. Basically, when there is no entity defined in xml, we still need a kind of stub to represent the empty environment/group when merging partials.

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 1, 2019

Do we want to remove the schema bump then? And display either the full remote scm config or not display any of the remote configs?

@tomzo
Copy link
Member

@tomzo tomzo commented Feb 1, 2019

Do we want to remove the schema bump then? And display either the full remote scm config or not display any of the remote configs?

If the direction is to remove xml page in the future, then I guess there is no point in extending it now. I would not display any of the remote config. It only matters to return the entities via API.

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 1, 2019

@arvindsv thoughts?

@arvindsv
Copy link
Member

@arvindsv arvindsv commented Feb 1, 2019

My preference would be to not change the XML at all. I remember there was some bug which caused Jyoti to finally leave the stub in the XML. @tomzo is right that it was around the merge.

  1. I think it's valuable to go back and look at why that was, if possible. I can try and take a look too, a little later though.

  2. If there's no reason to show it in the XML, then let's show nothing in the XML. At runtime, after a merge, it'll be present and used everywhere. But, the XML shouldn't really show it.

About getting rid of the XML completely ... it's a dream. :) Let's chip away at it, but removing it completely is going to be tough in the short term.

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 4, 2019

@tomzo @arvindsv This is the PR that added the environment and pipeline group stubs to the xml #2256

@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 4, 2019

It doesn't look like we need the scm stubs in the xml. I'll remove them unless there are any objections

@tomzo @arvindsv

EDIT: one reason is that the scm id will go out of sync after a server restart.

@ibnc ibnc changed the title WIP: Adding the ability to add SCMs through config repo Adding the ability to add SCMs through config repo Feb 4, 2019
@ibnc
Copy link
Contributor Author

@ibnc ibnc commented Feb 4, 2019

@tomzo @arvindsv if everything looks good to y'all, then would one of you mind merging this?

@tomzo tomzo merged commit a99f384 into gocd:master Feb 4, 2019
8 checks passed
@maheshp maheshp moved this from In progress to Done in 19.2.0 Feb 5, 2019
@rajiesh rajiesh moved this from Done to QA Done in 19.2.0 Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
19.2.0
QA Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants