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 script to sync from OpenDaylight (#556) #557

Closed

Conversation

vorburger
Copy link
Contributor

@vorburger vorburger commented Jan 22, 2019

see #556

Copy link

@skitt skitt left a comment

Choose a reason for hiding this comment

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

Wouldn’t it be better to clone autorelease in a temporary directory, so we can avoid the resets etc.? Also, how should the script deal with duplicates?

mkdir experimental/odp

# There is a probably a better way to do this, but this works... ;-)
find /home/vorburger/dev/ODL/git/releng/autorelease -name "*.yang" -path "*src/main/yang*" ! -path "*test*" -printf 'cp %p experimental/odp/%f\n' >/tmp/copy-yang.sh
Copy link

@skitt skitt Jan 23, 2019

Choose a reason for hiding this comment

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

find $1 -path "*/src/main/yang/*.yang" \! -path "*test*" -exec cp {} experimental/odp/ \;

or, if we can assume GNU cp:

find $1 -path "*/src/main/yang/*.yang" \! -path "*test*" -exec cp -t experimental/odp/ {} +

@tnadeau
Copy link
Contributor

tnadeau commented Jan 23, 2019 via email

@vorburger
Copy link
Contributor Author

Can you give the Travis/CI a run with this to see how it reacts?

can I suggest that we go incrementally and step by step:

1st step: First let's just get this script reviewed and merged, this lets anyone easily run the update locally, not yet with CI. I can push an update to incorporate the feedback from @skitt (thanks!), but perhaps it would be useful to first get you guys (committers on this project) to review #558 - is that OK and what you were hoping for? So please merge that one first to 'acknowledge' that the output which this script produced is what you really wanted...

2nd step: separately after this is accepted see how this script can be called from a CI... the problem here IMHO is which CI to use ... Travis CI, AFAIK, can only react to commits in this repo. But what I guess you guys here really want is a regular scheduled thing, to run the script proposed here once a week? I don't think Travis will do that.. we need some CI system somewhere else to do that. Let's pick that up after this is merged?

@tnadeau
Copy link
Contributor

tnadeau commented Jan 24, 2019 via email

@vorburger
Copy link
Contributor Author

Do we really need #558 <#558> ? It looks like you copied over all of the latest yang files from ODL and will simply overwrite what is there. Can’t we just invoke the script changes from #557 <#557> to do this?

So #558 is what the script proposed in this PR here (#557) produces when it runs (wherever it's run from). I ran this script locally, and it produces #558. I'm suggesting that, before looking into CI/CD, you confirm that #558 is indeed what you guys were hoping for here? If yes, then as a first step merge that already, so that at least you have more up to date models than the current, outdated years ago, already. If no, then reject the #558 PR saying what is wrong with it (e.g. too many models? propose a filter... Or prefer to keep sub-directories? Or whatever...) and then I'll update the script proposed here to adjust for it.

git submodule foreach git pull --ff-only origin master
popd

git rm experimental/odp/*
Copy link
Member

Choose a reason for hiding this comment

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

It seems a shame to lose the history of models as they evolve, as I think that is what will happen with this approach. I realize this is the easiest way to do it, but would you consider something that maintains version history for files that survive from one upload to the next? This could, for example, assist developers in adopting changes.

Copy link
Member

Choose a reason for hiding this comment

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

Further, do developers need to access release-specific instances of models from here? This is also made harder unless you have some form of tagging strategy associated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems a shame to lose the history of models as they evolve

@einarnn the history of models as they evolve is actually preserved like this, see e.g. this change over in the #558 example

Further, do developers need to access release-specific instances of models from here? This is also made harder unless you have some form of tagging strategy associated.

This script just automated what is currently in experimental/odp, which is a flat list of *.yang files which someone manually put there eons again. If you guys would like to have it release specific, that's certainly doable, but the best way to do that would perhaps would best be discussed in a separate issue - would you like to open one? Once there is full agreement on what the best strategy is, then this script could of course be adjusted accordingly. (I guess one option would be to have sub-directories, like experimental/odp/[oxygen|fluorine|neon|master], but that actually makes tracking history (above) harder and no easier... another option could be to git tag revisions in this repo with the revision in the git.opendaylight.org repo... the problem is that they are project specific there, so that's perhaps less trivial to automate and script, and probably useless or at least very confusing for the current completely flat list of YANG files under experimental/odp ... perhaps it should be arranged as experimental/odp/netvirt, experimental/odp/genius etc. with project specific directories instead...

Copy link

@skitt skitt Jan 28, 2019

Choose a reason for hiding this comment

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

It seems a shame to lose the history of models as they evolve

@einarnn the history of models as they evolve is actually preserved like this, see e.g. this change over in the #558 example

I suspect @einarnn would like to have each YANG model’s history preserved, i.e. the commit logs from each .yang file, as you would get by using filter-branch and a merge with unrelated histories. Syncing regularly as done in the current script shows the evolution of each model, but loses the commit messages and thereby the reasons for the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skitt thanks, I see what you mean, I think. @einarnn can we clarify what you / this project are after? My current script does preserve the history in this repo, see this example. It does not "preserve" the history from the "source" ODL Git repo in this repo, so e.g. on that flow-topology-discovery.yang example you don't get the git log from the ODL Git repo for that file... I wrote preserve in quote because that more of a ... "transposition" than a "preservation" - from one git repo to another. Doing that is beyond what I am both capable, interested in and have time for here - sorry! If that is the requirement, then what I am suggesting here is the wrong approach; should that be a hard requirement (and doing this as a first step is not of interest to this community), then I would abandon this PR and let someome else step up to contribute a better alternative instead.

Copy link
Member

@einarnn einarnn left a comment

Choose a reason for hiding this comment

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

Could you perhaps look at my comment on maintaining version history of the models?

@tnadeau
Copy link
Contributor

tnadeau commented Jan 28, 2019 via email

@tnadeau
Copy link
Contributor

tnadeau commented Jan 28, 2019 via email

@einarnn
Copy link
Member

einarnn commented Feb 8, 2020

Has been open too long. Please re-open if necessary.

@einarnn einarnn closed this Feb 8, 2020
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