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

AAKaaS:1 check Component Versions #2023

Merged
merged 86 commits into from
Sep 17, 2020
Merged

Conversation

tiloKo
Copy link
Member

@tiloKo tiloKo commented Sep 15, 2020

Changes

  • Tests
  • Documentation

As discussed for PR #1973 -> Here the first of a series of PRs for the AAKaaS steps

@DanielMieg
Copy link
Member

/it

Copy link
Member

@DanielMieg DanielMieg left a comment

Choose a reason for hiding this comment

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

Looks good overall. What do you think about the default?

Copy link
Member

@CCFenner CCFenner left a comment

Choose a reason for hiding this comment

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

Some remarks..

Please provide a speaking title for the PR.

documentation/docs/steps/abapAddonAssemblyKitCheckCVs.md Outdated Show resolved Hide resolved
CV *cv `json:"d"`
}

type cv struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth extracting cv and it's functions also to the abap package?

Copy link
Member

Choose a reason for hiding this comment

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

BTW: what does cv stand for?

Copy link
Member Author

Choose a reason for hiding this comment

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

cv is the acronym for "(software)component version" which are the (more or less) independent deployable units of an ABAP system.
Honestly I'm new to go and do not know where to place the coding best. As far as I got the cv struct is only used in this step without reuse in any of the others - that's why I have not extracted it. Anyway what I read so far about recommended directory structure I was a bit surprised there is no "internal" package. So what's the suggestion in this case? Should it be extracted even if there is only on step using it?

resources/metadata/abapAddonAssemblyKitCheckCVs.yaml Outdated Show resolved Hide resolved
description: Credential stored in Jenkins for Addon Assembly Kit as a Service System (AAKaaS)
type: jenkins
params:
- name: abapAddonAssemblyKitEndpoint
Copy link
Member

Choose a reason for hiding this comment

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

Is this a full qualified url or just the endpoint? For URL we typically call the parameter serverUrl.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a bit tricky - what we need as input for this parameter is:

  • In URI terms we just need scheme and authority - not the path
  • in URL terms we just need the scheme, host and port - not the path,
  • the WSDL term endpoint refers usually just to the URL including the path which we do not need as we have the path hard coded because every step uses a different path and we just wanted to have a single parameter instead of 7 different ones - especially as all 7 steps must be called in the same AAKaaS "system"
    ... so all did not match 100%...

- STAGES
- STEPS
- GENERAL
- name: addonDescriptor
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea, but can addonDescriptor and addonDescriptorFileName be combined that they both use the same format and thus we only need one parameter? It sounds as it contains the same information.

Copy link
Member Author

Choose a reason for hiding this comment

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

As we plan an refactoring anyway I would take this as action item for an later PR

@DanielMieg
Copy link
Member

I just noticed that that only the golang implementation is part of this PR. The groovy part for this step is missing. IMO, this approach is fine - the groovy part can be done in a later PR.

@tiloKo
Copy link
Member Author

tiloKo commented Sep 16, 2020

I added now the groovy part as well as all the insertions into general files (groovy, go, doku) which have to be done for every again and again :-/

@DanielMieg
Copy link
Member

I added now the groovy part as well as all the insertions into general files (groovy, go, doku) which have to be done for every again and again :-/

Or in the last PR for all steps together

@DanielMieg
Copy link
Member

/it

@tiloKo
Copy link
Member Author

tiloKo commented Sep 16, 2020

Or in the last PR for all steps together

If we follow the process in the most accurate way the content should match ;-)

@DanielMieg
Copy link
Member

/it

@tiloKo tiloKo changed the title Aa kaa s 1 check c vs AAKaaS:1 check Component Versions Sep 16, 2020
Copy link
Member

@DanielMieg DanielMieg left a comment

Choose a reason for hiding this comment

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

👍

@DanielMieg
Copy link
Member

/it

@DanielMieg DanielMieg merged commit 2a776ba into SAP:master Sep 17, 2020
@tiloKo tiloKo deleted the AAKaaS_1_checkCVs branch September 17, 2020 09:14
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