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.

Sign up for GitHub

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

Jump to bottom

Various changes & refactoring #6

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

Various changes & refactoring #6

wants to merge 12 commits into from

Conversation

aerostitch
Copy link
Contributor

@aerostitch aerostitch commented Apr 4, 2018 •
edited
Loading

This PR:

  • adds the structs related to pipeline config objects
  • changes getJSON to be able to return an etag header coming from the server
  • renames the methods I created in the previous days to make them easier to find in the doc and regroup them logically
  • adds a method to get a given pipeline configuration
  • Refactor agent.go to use the getJSON method
  • Refactor environment.go to use the getJSON method
  • Refactor GetJobHistory to use the getJSON method
  • Refactor GetPipelineGroups to use the getJSON method

@aerostitch aerostitch changed the title [WIP] Pipeline config-related changes Pipeline config-related changes Apr 5, 2018
@aerostitch
Copy link
Contributor Author

I'm done with this PR, you can go ahead and review it.
Thanks! :)

@aerostitch aerostitch changed the title Pipeline config-related changes Various changes & refactoring Apr 5, 2018
ashwanthkumar
ashwanthkumar reviewed Apr 7, 2018
View reviewed changes
client.go Outdated
PausePipeline(string, string) (*SimpleMessage, error)
UnpausePipeline(string) (*SimpleMessage, error)
UnlockPipeline(string) (*SimpleMessage, error)
PipelineGetInstance(string, int) (*PipelineInstance, error)
Copy link
Owner

Choose a reason for hiding this comment

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

I'm under the impression that methods would generally have to start with an action word. Like Get / Pause / Unlock. Is there a reason we're changing that and moving towards Pipeline as a prefix?

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 was to have them regrouped in the doc (all the pipeline-related functions together for example) to make them easier to find. But I can change it back.

@aerostitch
Copy link
Contributor Author

@ashwanthkumar I changed the name of the functions back to start with the action.

@ashwanthkumar
Copy link
Owner

@aerostitch Thanks for that. I would like to setup the automated build for this project. Can you please give me a few days, until I get that done and I would merge this in? Would that be okay?

@aerostitch
Copy link
Contributor Author

Sure, no problem, go ahead.

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

@ashwanthkumar ashwanthkumar ashwanthkumar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aerostitch @ashwanthkumar