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

pkglistgen: migrate bash scripts to python. #1312

Merged
merged 3 commits into from Jan 2, 2018

Conversation

@jberry-suse
Copy link
Contributor

jberry-suse commented Dec 21, 2017

Per @lnussel email request to rewrite wrapper scripts to python.

There are likely issues since I have not looked at SLE scripts, but presumably they can be merged on top of this as I would expect only additional config options if anything.

The ports setup will likely need some love for Tumbleweed, but it did already so pre-existing. I did improve it by moving options to project config. The ports run is included in the all scope and thus run by service file. If that is not desired it should just be removed from the all scope.

Need to check to see that Package.commit() handles no changes gracefully.

Corrected lots of hard-coded lines with real code as would be needed for Tumbleweed and likely SLE scripts contain a different version already. This stuff literally takes longer to write this way (ie multiple times) than properly.

: ${letter:=A B C D E}
# ...
	if [ "$l" != A -a "$l" != B ]; then
		repos="$repos,$project/bootstrap_copy"
	fi

I wrote a proper is_staging_bootstrapped() function for request_splitter.py so it was an easy drop in.

In the future it would be much appreciated if things were done cleanly in the first place as cleaning up afterwards is many times harder, rather unpleasant, and overall wastes much more time than had it been done right originally. Additionally, it requires loading the entire problem space into one's mind and essentially re-solving the original problem. Plus lots and lots of squinting at crap like this that is nearly identical with minor differences that should have been a function.

echo '<multibuild>' > _multibuild
for file in *.kiwi; do
	container="${file##*/}"
	container="${container%.kiwi}"
	echo "  <package>${container}</package>" >> _multibuild
done
echo '</multibuild>' >> _multibuild
cat << EOF > stub.kiwi
# prevent building single kiwi files twice
Name: stub
Version: 0.0
EOF
checkin

if [ -z "$skip_releases" ]; then
	cd "$cachedir/$releases"
	echo '<multibuild>' > _multibuild
	for file in *.spec; do
		container="${file##*/}"
		container="${container%.spec}"
		echo "  <package>${container}</package>" >> _multibuild
	done
	echo '</multibuild>' >> _multibuild
	cat <<-EOF > stub.spec
	# prevent building single spec files twice
	Name: stub
	Version: 0.0
	EOF
	checkin
fi

Which becomes:

self.multibuild_from_glob(product_dir, '*.kiwi')
self.build_stub(product_dir, 'kiwi')
self.commit_package(product_dir)

if not skip_release:
    self.multibuild_from_glob(release_dir, '*.spec')
    self.build_stub(release_dir, 'spec')
    self.commit_package(release_dir)

For now I have given up on test efforts until I have time to resolve the larger travis environment issues.

The entire thing could use a good once-over and testing to ensure the previous functionality still works as I have only run target without full mirroring and such.

@jberry-suse jberry-suse requested a review from lnussel Dec 21, 2017
@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

The config options should also be configurable via remote config which may be preferable.

@jberry-suse jberry-suse force-pushed the jberry-suse:pkglistgen-wrapper-nuke branch from c5f9cda to 5e9392f Dec 21, 2017
@openSUSE openSUSE deleted a comment from coveralls Dec 21, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Dec 21, 2017

Coverage Status

Coverage decreased (-0.4%) to 29.535% when pulling 5e9392f on jberry-suse:pkglistgen-wrapper-nuke into ba8a357 on openSUSE:master.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Dec 21, 2017

SLE15: http://paste.suse.de/18039
SLE15 rings: http://paste.suse.de/18043
SLE15 stagings: http://paste.suse.de/18047

I don't think I'll be able to map that into your python framework within reasonable time :(

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

It does not look all that different. A couple extra flags are set on pkglistgen.py calls as expected which would need to be configurable via osclib/conf.py just like I did for Leap. The status checks and _multibuild generation all seems the same and presumably --skip-releases works the same. I'll need to look more closely, but skimming tells me this is what I expected.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

Rather than hard-coding all the ring and staging mess osclib/conf.py and osclib/stagingapi.py are relied upon which can handle that generically.

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Dec 21, 2017

Of course they don't look all that different - they are the grandparents :)

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

As I assumed, but then I do not understand the comment about not being able to map them in a "reasonable time".

@coolo

This comment has been minimized.

Copy link
Member

coolo commented Dec 21, 2017

@lnussel

This comment has been minimized.

Copy link
Member

lnussel commented Dec 21, 2017

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

@coolo So a reference to not being able to finish before vacation, based on your schedule, rather than how long the task will take.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

Usually the process would have stopped here but now that we
have someone who can do it properly and in a more future proof way
we can hand over to: You :-)

Even if I write very rough spaghetti code I always clean it up before pushing. It is much easier, even several days later, to remember that I copy/pasted something or when reading through code where and why I did something that needs to be cleaned up than it is for a third-party to a) detect that something was copied, and b) how to un-copy it.

One tends to always have vague requirements along the lines of "make it work" when doing creative work even if a larger plan is fairly well defined. If the code is worth writing it is worth writing well. Virtually everything I have been asked to do was very vague, but you don't see this sort of code from me. If not cleaned up during development process or as an explicit task such as what I am doing it likely never will be cleaned up. Then people keep adding on to it and because it is not clean go ahead and copy/paste further. I had numerous instances of code I would find with slight variations in this repo 5-10 times repeated. It was rather a PITA to derive a common function from that and rewire the code to use it just so I could build atop it cleanly. On the other hand it is very easy for the second person to need that code to move it to a general location and add one parameter if even needed. That is the proper "iterative" approach, not an excuse for never cleaning things up.

This is what is meant by technical debt as it gets worse over time not better and is why not introducing it is extremely important. It makes future work less than pleasant and more likely to introduce more debt or spend a large amount of time rewriting things. In the end double, triple, or more is spent on the same problem space instead of the added ~20% time, if that, of the original author who is already familiar with it.

The staging-bot code was a complete unknown other than I had a vague idea of what I wanted it to do after working with the staging tools, but you don't see copy/pasted code and a set of wrapper scripts per project for it.

To be clear, I rarely formalize things at the start and vastly prefer an iterative approach after a round of high-level design. Many times I go through several iterations of sections of the code before I am happy as you typically spot areas for improvement as you work. The key here, is not skipping the last step to the "iterative" process...the final polish. Just because it runs does not mean it should be pushed.

Obviously, there will always be cases where new requirements or technology comes along and things need to be re-written, but there is no reason to have things in this sort of state. Not to mention, more readable code makes it far easier to re-write. Personally, I find this sort of task extremely frustrating given the amount of time I must spend just to make the code keep doing what it did before and the fact that it is 100% avoidable. A lot more impactful things already detailed in issues and even poked about repeatedly that I can imagine everyone would rather I work on.

@jberry-suse

This comment has been minimized.

Copy link
Contributor Author

jberry-suse commented Dec 21, 2017

It's not like supporting the 3 main targets is a surprise requirement.

@lnussel lnussel merged commit 95ec1f4 into openSUSE:master Jan 2, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jberry-suse jberry-suse deleted the jberry-suse:pkglistgen-wrapper-nuke branch Jan 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.