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

[POC] Super nasty rendering of jobs that needs manual triggering #3163

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@simonjohansson
Copy link
Contributor

simonjohansson commented Jan 29, 2019

Following discussion in https://discuss.concourse-ci.org/t/improved-ui-for-manually-triggered-jobs/146 and #2233 here is a naive stupid implementation of what we need.

No tests, absolutely no architectural consideration, no understanding of Elm so all logic is in the api and in the JS. Just meant as a POC to get the ball rolling.

Here is a video that shows it off with a brief explanation
https://vimeo.com/313990031

@simonjohansson simonjohansson force-pushed the simonjohansson:manual-trigger branch from c6f6fc4 to 20a425e Jan 29, 2019

@marco-m

This comment has been minimized.

Copy link

marco-m commented Jan 29, 2019

@simonjohansson video link is broken

@vito

This comment has been minimized.

Copy link
Member

vito commented Jan 29, 2019

Neat, this would definitely be a great feature to have!

I do have to warn ya that we've redesigned the pipeline UI for Spaces (#1707), so these changes will likely be shortlived if they're mostly in the current JS frontend.

I also noticed one odd thing in the POC - the job downstream of b was marked as 'needs-kicking' even though its upstream dependency on b was not yet satisfied. I think we'd need this to be integrated into the API somehow to communicate this more easily to the frontend.

Maybe the scheduler could mark the job as 'triggerable' somewhere here:

inputMapping, err := s.InputMapper.SaveNextInputMapping(logger, versions, job, resources)
if err != nil {
return err
}
for _, inputConfig := range job.Config().Inputs() {
inputVersion, ok := inputMapping[inputConfig.Name]
//trigger: true, and the version has not been used
if ok && inputVersion.FirstOccurrence && inputConfig.Trigger {
err := job.EnsurePendingBuildExists()
if err != nil {
logger.Error("failed-to-ensure-pending-build-exists", err)
return err
}
break
}
}
return nil

It could loop over the inputs, and if any of them have FirstOccurrence as true and none of them were marked trigger: true, it could mark the job as 'needs kick'. Then the API could just surface this as a boolean property in the job JSON.

This kind of work would probably still be relevant once 'spaces' ships - maybe we could start on that now and still implement something simple in the current web UI? It'll be easier to transfer it over to the new UI with most of the work done in the backend.

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Jan 29, 2019

@marco-m thanks for the spot, updated the link :)

@vito

the job downstream of b was marked as 'needs-kicking' even though its upstream dependency on b was not yet satisfied.

Implementation is full of these kind of bugs :) It would be preferable to as you say have this resolving further down the stack.

Maybe the scheduler could mark the job as 'triggerable' somewhere here:

Thanks for the pointer, Ill see if I can do something more clever tomorrow!

@marco-m

This comment has been minimized.

Copy link

marco-m commented Jan 30, 2019

@simonjohansson I watched the video. Very nice!
I was thinking, a test you might want to do is to validate that it behaves as expected also if a "new" version is pinned.

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Jan 30, 2019

Is there any upcoming UI work to show what versions of a jobs resources would be used if a user would trigger it manually? Wondering if we need send more data over the wire that just needs-kick

@vito

This comment has been minimized.

Copy link
Member

vito commented Jan 30, 2019

@simonjohansson Nothing planned soon, but we do have another issue for precisely that: #269

The information is actually available in the code that I linked (inputMapping), so it could be neat to expose that through the API, too. You could even show what's new by exposing the 'FirstOccurrence` field.

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Jan 31, 2019

@vito before moving on, there is two approaches we could take.

  1. Clean up and test the POC, i.e just expose the data we need to the frontend and then do all the logic there.

  2. Instead of passing along all the necessary data to the frontend just compute a value on the backend. something like:

// Again just POC, this needs to be tested, put in the correct place etc. etc.
func (s *Server) ListJobs(pipeline db.Pipeline) http.Handler {
		dashboard, err := pipeline.Dashboard()
		resources, _ := pipeline.Resources()
....
		graph := CreateGraph(dashboard, resources)

		for _, job := range dashboard {
			pj := present.Job(
				teamName,
				job.Job,
				job.FinishedBuild,
				job.NextBuild,
				job.TransitionBuild,
			)

			pj.NeedsKick =  NeedsAManualTrigger(job.Job.Name(), graph)
			jobs = append(jobs, pj)
		}
....
}

func NeedsAManualTrigger(jobName string, graph Graph) bool {
	//Magic
	return false
}

func CreateGraph(dashboard db.Dashboard, resources db.Resources) (graph Graph) {
	for _, resource := range resources {
                 // Make sure to take pins into account...
		v, _, _, _ := resource.Versions(db.Page{Limit: 1})
		graph.Resources[resource.Name()] = v[0]
	}

	for _, job := range dashboard {
		lastSuccessfull, _, _ := job.Job.LastSuccessfulBuild()
		jobInputs := job.Job.Config().Inputs()
		buildInputs, _, _ := lastSuccessfull.Resources()

		graph.Jobs[job.Job.Name()] = GraphJob{
			JobInput:                  jobInputs,
			LastSuccessfulBuildInputs: buildInputs,
		}
	}

	return
}

type GraphJob struct {
	// Configured inputs for a job, contains relationships with other jobs
	// and if the build is manually triggered
	JobInput                  []atc.JobInput

	// Contains information about what versions the last successful build was run with
	LastSuccessfulBuildInputs []db.BuildInput
}

type Graph struct {
	Resources map[string]atc.ResourceVersion
	Jobs      map[string]GraphJob
}

We would not expose any data that we might need in the future, but then again, why would we, as we haven't started implementing those features yet :)

Personally I would prefer option 2

  • JS Frontend will change as you mentioned above so having a single value determine a class on a div would probably be easier to refactor than logic and values relying on data from multiple api calls
  • JS is not unit tested atm(so we would need to add a test framework around it), and after poking a bit in the Elm code, it looks like that code is more interested in displaying data rather than mangling it(i.e determining if a job needs a kick or not).
  • Im more comfortable with testing/developing Go than anything on the frontend.
@vito

This comment has been minimized.

Copy link
Member

vito commented Feb 6, 2019

@simonjohansson The main problem is there's no way to know whether a job needs triggering from that part of the code, either. You could guess, which I think is fine for a proof-of-concept while figuring out the UI/UX, but I wouldn't want to ship it if it's misleading in some cases. As far as I can tell the knowledge of whether a job needs triggering is only available for a fleeting moment in the scheduler.

I think it wouldn't be too hard to just add a bool value on the job, which could be set in that part of the scheduler I linked. That'll be easier than exposing the actual inputs in the API - we can cross that bridge when we get to it. 🙂 You'd just need a migration to add the column to the jobs table and then a method to set it, called by the scheduler.

Happy to help along via PR reviews if you want to give it a go!

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Feb 12, 2019

@vito thanks for the pointers, was a bit afraid of doing such an intrusive change, but with your blessing Ill look into it :)

@simonjohansson simonjohansson force-pushed the simonjohansson:manual-trigger branch 2 times, most recently from d41d8d3 to 5717562 Feb 21, 2019

Adding needs manual trigger support in the api
Signed-off-by: Simon Johansson <simon@simonjohansson.com>

@simonjohansson simonjohansson force-pushed the simonjohansson:manual-trigger branch from 5717562 to bfe6598 Feb 21, 2019

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Feb 21, 2019

@vito took some time, holiday and stuff, but Ive updated the POC on the API side with your suggestions with @stigtermichiel , please have a look ❤️

@simonjohansson

This comment has been minimized.

Copy link
Contributor Author

simonjohansson commented Feb 21, 2019

Regarding the failing ci task,

pr added migrations:
1550666656_add_needs_kick_to_jobs.down.sql
1550666656_add_needs_kick_to_jobs.up.sql
out of order with master.
new migrations in a pr must be strictly newer than those on master.

It seems to be whitespace related, if running

diff -w "$expected_pr_migrations" <(sort -n "$actual_pr_migrations") >/dev/null || {
...

instead of

diff "$expected_pr_migrations" <(sort -n "$actual_pr_migrations") >/dev/null || {
...

in check-migration-order

OR

echo "$new_on_pr" >> "$expected_pr_migrations"

instead of

echo -n "$new_on_pr" >> "$expected_pr_migrations"

The test passes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment