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

Additional colours with cyclic usage #2

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

cinek810
Copy link
Contributor

@cinek810 cinek810 commented Aug 6, 2014

Very simple but maybe will help someone trying to monitor more partitions than predefined colours in array.

@jabrcx
Copy link
Contributor

jabrcx commented Sep 6, 2014

Thanks for the patches! I hesitated when I saw this because of the additional PIL imports that I'd like to avoid requiring by default, but now I see this is a different issue:

There are commits here for both the cyclic color picking and removing the JobScript/JobScriptPreview text from the report, replacing it with an image. Is that intentional? If so, could you or @fafik23 explain what's gained and/or lossed (i.e. hidden) with the text-to-image code? Even if it was included unintentionally, maybe it's something good for the mainline. Best if you could separate the color-picking and text-to-image issues into branches that can be dealt with independently, though I see now this is almost all the latter.

Actually, the text-to-image patches deal with lines that have since changed. Since it's taken me a while to deal with this pull request, I can help with the merging once I understand what the goal is.

Thanks!

@cinek810
Copy link
Contributor Author

cinek810 commented Sep 9, 2014

Cyclic colours are in two first patches only - they deal only with one file.

text-to-image conversion was done by @fafik23 because we considered a security risk that someone can introduce for example a malicoius java script code into job script so it will be executed on administrators work station (we now it's a bit funny).

@jabrcx
Copy link
Contributor

jabrcx commented Sep 12, 2014

Cool, I'll merge in the color code , and I've created issue #7 to make sure code injection is confidently prevented one way or another. Thanks for the insight!

@jabrcx
Copy link
Contributor

jabrcx commented Sep 22, 2014

I've added the extra colors.

Patch ed849a2 in this pull request (which deals with the possible $colours array overrun introduced in f273763 in #1) is already fixed by the earlier patch 92acef2 when I merged in #1. I chose to re-use the last color rather then loop back through from the beginning, so that partitions can be specified in order of interest, and nothing will re-use the most relevant partitions' colors (which would make the graph particularly unreadable). In other words, the current code segregates any color clobbering to the end of the partition list. Hope that works for you, too.

Thanks!

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.

3 participants